oslat: Fix --cpu-list won't allow to schedule on all possible cores

parse_cpumask() is too strict for oslat, in that use_current_cpuset() will filter out all the cores that are not allowed for current process to run. This seems to be unnecessary at least for oslat. For example, the bash process that runs the oslat program may have a sched affinity of 0-2, however it's still legal to have it start a oslat thread running on the cores outside 0-2 as long as the follow up sched_setaffinity() will succeed.

numa_parse_cpustring_all() suites exactly for this case, which should already have considered sysconf(_SC_NPROCESSORS_ONLN) limit. Use that instead.

Since at it, also remove initialization of cpu_set variable otherwise it's leaked in previous parse_cpumask too: numa_parse_cpustring_all() will return a newly allocated buffer already. Quotting from manual:

numa_parse_nodestring() parses a character string list of nodes into a bit mask. The bit mask is allocated by numa_allocate_nodemask().

numa_parse_nodestring_all() is similar to numa_parse_nodestring, but can parse all possible nodes, not only current nodeset.

Hi Peter,

Quoted 14 lines

parse_cpumask() is too strict for oslat, in that use_current_cpuset() will filter out all the cores that are not allowed for current process to run. This seems to be unnecessary at least for oslat. For example, the bash process that runs the oslat program may have a sched affinity of 0-2, however it's still legal to have it start a oslat thread running on the cores outside 0-2 as long as the follow up sched_setaffinity() will succeed.

numa_parse_cpustring_all() suites exactly for this case, which should already have considered sysconf(_SC_NPROCESSORS_ONLN) limit. Use that instead.

Since at it, also remove initialization of cpu_set variable otherwise it's leaked in previous parse_cpumask too: numa_parse_cpustring_all() will return a newly allocated buffer already. Quotting from manual:

Quoted 5 lines

numa_parse_nodestring() parses a character string list of nodes into a bit mask. The bit mask is allocated by numa_allocate_nodemask().

numa_parse_nodestring_all() is similar to numa_parse_nodestring, but can parse all possible nodes, not only current nodeset.

My 2 cents: If parse_cpumask() is to strict fix parse_cpumask() so all tools do the same thing. Note parse_cpumask() contains a couple of bugs:

"rt-numa: Use mask size for iterator limit" "rt-numa: Remove max_cpus argument from parse_cpusmask"

Yes I explicitly avoided touching parse_cpumask because I don't want to change behavior of other tools if I'm not confident with that. Would above two patches fix oslat too (which I didn't check)? If so, I'll be fine to have this patch dropped. Otherwise I tend to prefer fixing oslat first. We can further rework the common code, but if existing tools are fine, then I don't think it's a bugfix, so no need to rush. While I'll count this patch as a real bugfix, so I'd hope we could consider merging it earlier.

Quoted 7 lines · Peter Xu

Yes I explicitly avoided touching parse_cpumask because I don't want to change behavior of other tools if I'm not confident with that. Would above two patches fix oslat too (which I didn't check)? If so, I'll be fine to have this patch dropped. Otherwise I tend to prefer fixing oslat first. We can further rework the common code, but if existing tools are fine, then I don't think it's a bugfix, so no need to rush. While I'll count this patch as a real bugfix, so I'd hope we could consider merging it earlier.

As I said, I would really appreciated if all tools behave the same way. Having oslat be special is going to be pain in the long run. Just update parse_cpumask, it's not that difficult :)

Now I tried to fix it but it seems you don't really like my fix.

Would you propose yours instead? I would be more than glad if your version is better then I'm happy to drop mine. Otherwise I'll still prefer to have this patch merged to unbreak it first.

Hi Peter,

Quoted 1 line · Peter Xu

Now I tried to fix it but it seems you don't really like my fix.

Sorry I didn't want to say I don't like your fix. I just said, I would appreciate if things would stay consistent. But then it seems there is not much interest in this.

Quoted 3 lines

Would you propose yours instead? I would be more than glad if your version is better then I'm happy to drop mine. Otherwise I'll still prefer to have this patch merged to unbreak it first.

I don't care too much. If this patch fixes your problem don't let me stop you.

Daniel,

Quoted 9 lines · Daniel Wagner

Quoted 7 lines · Peter Xu

Yes I explicitly avoided touching parse_cpumask because I don't want to change behavior of other tools if I'm not confident with that. Would above two patches fix oslat too (which I didn't check)? If so, I'll be fine to have this patch dropped. Otherwise I tend to prefer fixing oslat first. We can further rework the common code, but if existing tools are fine, then I don't think it's a bugfix, so no need to rush. While I'll count this patch as a real bugfix, so I'd hope we could consider merging it earlier.

Quoted 3 lines

As I said, I would really appreciated if all tools behave the same way. Having oslat be special is going to be pain in the long run. Just update parse_cpumask, it's not that difficult :)

Oslat is broken now after the numa rework. Frankly I'm surprised it's broken so quickly with just a few commits and actually with no new feature at all but pure cleanups. Again I still appreciate all your work on this but I don't really appreciate a lot for having it broken..

Now I tried to fix it but it seems you don't really like my fix.

Would you propose yours instead? I would be more than glad if your version is better then I'm happy to drop mine. Otherwise I'll still prefer to have this patch merged to unbreak it first.

Thanks,

Signature · 1 line

Peter Xu

Hi Peter,

Quoted 14 lines · Peter Xu

parse_cpumask() is too strict for oslat, in that use_current_cpuset() will filter out all the cores that are not allowed for current process to run. This seems to be unnecessary at least for oslat. For example, the bash process that runs the oslat program may have a sched affinity of 0-2, however it's still legal to have it start a oslat thread running on the cores outside 0-2 as long as the follow up sched_setaffinity() will succeed.

numa_parse_cpustring_all() suites exactly for this case, which should already have considered sysconf(_SC_NPROCESSORS_ONLN) limit. Use that instead.

Since at it, also remove initialization of cpu_set variable otherwise it's leaked in previous parse_cpumask too: numa_parse_cpustring_all() will return a newly allocated buffer already. Quotting from manual:

Quoted 5 lines

numa_parse_nodestring() parses a character string list of nodes into a bit mask. The bit mask is allocated by numa_allocate_nodemask().

numa_parse_nodestring_all() is similar to numa_parse_nodestring, but can parse all possible nodes, not only current nodeset.

My 2 cents: If parse_cpumask() is to strict fix parse_cpumask() so all

tools do the same thing. Note parse_cpumask() contains a couple of bugs:

"rt-numa: Use mask size for iterator limit" "rt-numa: Remove max_cpus argument from parse_cpusmask"

Thanks,
Daniel

Quoted 2 lines

"rt-numa: Use mask size for iterator limit" "rt-numa: Remove max_cpus argument from parse_cpusmask"

Are those actually bug fixes or just a different way of doing things?

parse_cpumask() is too strict for oslat, in that use_current_cpuset() will filter out all the cores that are not allowed for current process to run. This seems to be unnecessary at least for oslat. For example, the bash process that runs the oslat program may have a sched affinity of 0-2, however it's still legal to have it start a oslat thread running on the cores outside 0-2 as long as the follow up sched_setaffinity() will succeed.

numa_parse_cpustring_all() suites exactly for this case, which should already have considered sysconf(_SC_NPROCESSORS_ONLN) limit. Use that instead.

Since at it, also remove initialization of cpu_set variable otherwise it's leaked in previous parse_cpumask too: numa_parse_cpustring_all() will return a newly allocated buffer already. Quotting from manual:

numa_parse_nodestring() parses a character string list of nodes into a bit mask. The bit mask is allocated by numa_allocate_nodemask().

numa_parse_nodestring_all() is similar to numa_parse_nodestring, but can parse all possible nodes, not only current nodeset.

Cc John Kacur <[email protected]>

Cc Daniel Wagner <[email protected]>

Cc Clark Williams <[email protected]>

Reported-by Pradipta Kumar Sahoo <[email protected]>

Reported-by Mike Stowell <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 src/oslat/oslat.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

src/oslat/oslat.c +2 -6

index b2c5373..465a694 100644
@@ -785,7 +785,6 @@ int main(int argc, char *argv[])
 	struct thread *threads;
 	int i, n_cores;
 	struct bitmask *cpu_set = NULL;
-	int max_cpus = sysconf(_SC_NPROCESSORS_ONLN);
 
 #ifdef FRC_MISSING
 	printf("This architecture is not yet supported. "

@@ -797,10 +796,6 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
-	cpu_set = numa_allocate_cpumask();
-	if (!cpu_set)
-		fatal("oslat: Could not allocate cpumask\n");
-
 	g.app_name = argv[0];
 	g.rtprio = 0;
 	g.bucket_size = BUCKET_SIZE;

@@ -817,7 +812,8 @@ int main(int argc, char *argv[])
 	if (!g.cpu_list)
 		g.cpu_list = strdup("all");
 
-	if (parse_cpumask(g.cpu_list, max_cpus, &cpu_set) != 0)
+	cpu_set = numa_parse_cpustring_all(g.cpu_list);
+	if (!cpu_set)
 		fatal("oslat: parse_cpumask failed.\n");
 	n_cores = numa_bitmask_weight(cpu_set);
 

Signature · 1 line

2.26.2