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

sched-simple: fix alloc/free for resource sets with equal size but different cpusets #2203

Merged
merged 2 commits into from Jun 21, 2019

Conversation

@grondo
Copy link
Contributor

commented Jun 21, 2019

This PR fixes #2202. The problem was a dumb implementation of an idset comparator internal to sched-simple's rlist object. The idset_cmp() function compared non-equal idsets using idset_count(), but of course two idsets can have the same count but still not be equivalent.

The new implementation detects unequal idsets that have the same "count" and then falls back to comparing the first different stored integer.

This appears to fix the weirdness under Slurm with cpu-affinity:

$ srun --pty -N32 -n128 src/cmd/flux start
$ 
$ src/modules/sched-simple/rlist-query 
rank[2,6,10,14,18,22,26,30,34,38,42,46,50,54,58,62,66,70,74,78,82,86,90,94,98,102,106,110,114,118,122,126]/core2 rank[1,5,9,13,17,21,25,29,33,37,41,45,49,53,57,61,65,69,73,77,81,85,89,93,97,101,105,109,113,117,121,125]/core1 rank[3,7,11,15,19,23,27,31,35,39,43,47,51,55,59,63,67,71,75,79,83,87,91,95,99,103,107,111,115,119,123,127]/core3 rank[0,4,8,12,16,20,24,28,32,36,40,44,48,52,56,60,64,68,72,76,80,84,88,92,96,100,104,108,112,116,120,124]/core0
$ flux srun -N128 hostname
$ echo $?
0
$ flux dmesg | tail
2019-06-21T16:47:13.705278Z sched-simple.debug[0]: req: 1069547520000: spec={128,128,1}
2019-06-21T16:47:13.705713Z sched-simple.debug[0]: alloc: 1069547520000: rank[0,4,8,12,16,20,24,28,32,36,40,44,48,52,56,60,64,68,72,76,80,84,88,92,96,100,104,108,112,116,120,124]/core0 rank[1,5,9,13,17,21,25,29,33,37,41,45,49,53,57,61,65,69,73,77,81,85,89,93,97,101,105,109,113,117,121,125]/core1 rank[3,7,11,15,19,23,27,31,35,39,43,47,51,55,59,63,67,71,75,79,83,87,91,95,99,103,107,111,115,119,123,127]/core3 rank[2,6,10,14,18,22,26,30,34,38,42,46,50,54,58,62,66,70,74,78,82,86,90,94,98,102,106,110,114,118,122,126]/core2
2019-06-21T16:47:13.718219Z kvs.debug[0]: aggregated 2 transactions (2 ops)
2019-06-21T16:47:13.723366Z sched-simple.debug[0]: free: rank[0,4,8,12,16,20,24,28,32,36,40,44,48,52,56,60,64,68,72,76,80,84,88,92,96,100,104,108,112,116,120,124]/core0 rank[1,5,9,13,17,21,25,29,33,37,41,45,49,53,57,61,65,69,73,77,81,85,89,93,97,101,105,109,113,117,121,125]/core1 rank[3,7,11,15,19,23,27,31,35,39,43,47,51,55,59,63,67,71,75,79,83,87,91,95,99,103,107,111,115,119,123,127]/core3 rank[2,6,10,14,18,22,26,30,34,38,42,46,50,54,58,62,66,70,74,78,82,86,90,94,98,102,106,110,114,118,122,126]/core2
unsigned int b = idset_first (set2);
while ((rv = (a - b)) == 0) {
a = idset_next (set1, a);
b = idset_next (set1, b);

This comment has been minimized.

Copy link
@chu11

chu11 Jun 21, 2019

Contributor

should be idset_next (set2)?

This comment has been minimized.

Copy link
@grondo

grondo Jun 21, 2019

Author Contributor

yep, thanks!

This comment has been minimized.

Copy link
@grondo

grondo Jun 21, 2019

Author Contributor

might have to quickly figure out how to ensure that code gets exercised, but I may not get to it...

This comment has been minimized.

Copy link
@grondo

grondo Jun 21, 2019

Author Contributor

@chu11, quickly fixed this issue with a new commit. Once you're done reviewing, I'll squash.

char *result = NULL;
struct rlist *a = NULL;

struct rlist *rl = rlist_from_hwloc_by_rank (R_issue2202);

This comment has been minimized.

Copy link
@chu11

chu11 Jun 21, 2019

Contributor

nit: used tabs instead of spaces

This comment has been minimized.

Copy link
@grondo

grondo Jun 21, 2019

Author Contributor

ugh, sorry about that. Somehow the editor I was using seems to have ignored modeline...

rlist_destroy (a);
}

rlist_destroy (rl);

This comment has been minimized.

Copy link
@chu11

chu11 Jun 21, 2019

Contributor

nit: used tabs instead of spaces

@@ -249,6 +317,7 @@ int main (int ac, char *av[])
run_test_entries (test_2n_4c, 2, 4);
run_test_entries (test_6n_4c, 6, 4);
run_test_entries (test_1024n_4c, 1024, 4);
test_issue2202 ();

This comment has been minimized.

Copy link
@chu11

chu11 Jun 21, 2019

Contributor

nit: used tabs instead of spaces

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

I'll also work on adding another testcase for ranks with cpusets with more than one core. That should exercise the idset_cmp() loop and get better coverage (and would have likely found the typo @chu11 discovered earlier)

@chu11

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

things look good to me, squash away

@grondo grondo force-pushed the grondo:issue#2202 branch from db1bc41 to 50c9cd7 Jun 21, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Thanks, squashed and added some extra tests to ensure the inner loop of idset_cmp is exercised. (I was going pretty quick here, I'll check back after lunch and give it another once over once I see coverage results)

grondo added 2 commits Jun 21, 2019
Problem: The internal idset_cmp implementation compares unequal
idsets using their current count, which obviously causes non-equal idsets
of the same count to appear equal. This breaks many functions that
rely on internal idset comparator.

Additionally, sorting idsets by the current count is probably not
what we want. Instead change idset_cmp to compare the first unequal
integer stored in the set (where the smaller set is considered lesser
when its contents match the first N items of a larger set)

Fixes #2202
Add an rlist unit test that exercises idset_cmp() comparison of
unequal idsets which are the same size.
@grondo grondo force-pushed the grondo:issue#2202 branch from 50c9cd7 to a984a93 Jun 21, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@chu11, actually I had a change of heart on idset_cmp(). It didn't seem logical to first sort sets based on idset_count() then the first non-matching integer id. This would sort the sets "0-1","10" as "10", "0-1". So instead I just changed idset_cmp() to first compare the first non-matching id from each set, where the smaller of the two sets would sort first if the first N ids of each set match.

Sorry, but can you review again? (You had some great finds last time)

I went ahead and squashed just in case the current changes are good and can be merged right away.

@codecov-io

This comment has been minimized.

Copy link

commented Jun 21, 2019

Codecov Report

Merging #2203 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2203      +/-   ##
==========================================
- Coverage   80.86%   80.84%   -0.03%     
==========================================
  Files         199      199              
  Lines       31815    31825      +10     
==========================================
  Hits        25728    25728              
- Misses       6087     6097      +10
Impacted Files Coverage Δ
src/modules/sched-simple/rlist.c 82.15% <100%> (+0.41%) ⬆️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/modules/barrier/barrier.c 78.52% <0%> (-2.02%) ⬇️
src/common/libflux/mrpc.c 87.79% <0%> (-1.19%) ⬇️
src/cmd/flux-module.c 83.72% <0%> (-0.24%) ⬇️
src/common/libflux/message.c 80.75% <0%> (-0.13%) ⬇️
@chu11

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@grondo looks good to me. Are you all done?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@grondo looks good to me. Are you all done?

Yep, thanks!!

@chu11 chu11 merged commit 075fa6d into flux-framework:master Jun 21, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.86%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +19.13% compared to 4c93728
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Thanks!

@grondo grondo deleted the grondo:issue#2202 branch Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.