Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: add test for external thread acuiring mutex #247

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

carns
Copy link
Member

@carns carns commented Feb 12, 2023

Add a test case to the scheduling unit test that requires an external thread (i.e. a pthread spawned prior to init) to acquire an argobots mutex as well to check CPU usage.

Something isn't right, though. Both the Argobots ULT and the pthread appear to consume CPU time. The former at least should have been corrected in pmodels/argobots#361.

Need to determine if this is a test program error or an Argobots regression. Should also add test cases for eventuals in addition to mutexes, if nothing else for a control case as those definitely are known not to busy spin.

@chuckcranor

@carns carns self-assigned this Feb 12, 2023
@carns
Copy link
Member Author

carns commented Feb 12, 2023

Since the fix for the busy spin problem has been available in argobots releases for some time now (it was present in 1.1) we should also alter the test logic to fail if CPU utilization is too high now. It was just displaying advisory information previously.

@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Merging #247 (8df0ced) into main (77bbe7a) will increase coverage by 0.29%.
The diff coverage is 94.64%.

@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
+ Coverage   54.18%   54.47%   +0.29%     
==========================================
  Files          65       65              
  Lines        8826     8910      +84     
  Branches     1156     1166      +10     
==========================================
+ Hits         4782     4854      +72     
- Misses       3356     3360       +4     
- Partials      688      696       +8     
Impacted Files Coverage Δ
tests/unit-tests/margo-scheduling.c 95.55% <94.64%> (-0.19%) ⬇️
src/margo-core.c 62.16% <0.00%> (-0.32%) ⬇️
include/margo.h 100.00% <0.00%> (ø)
src/margo-instance.h 100.00% <0.00%> (ø)
tests/unit-tests/margo-forward.c 97.52% <0.00%> (+0.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@carns
Copy link
Member Author

carns commented Feb 14, 2023

Busy spin problem was fixed in #249, but it still may be best to wait to rebase and merge this after resolving #250, which in turn is blocked on an Argobots point release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant