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

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

[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:

    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 <jkacur@redhat.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Clark Williams <williams@redhat.com>
Reported-by: Pradipta Kumar Sahoo <psahoo@redhat.com>
Reported-by: Mike Stowell <mstowell@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 src/oslat/oslat.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index b2c5373..465a694 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -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);
 
-- 
2.26.2

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

[Daniel Wagner]

Hi Peter,

On Thu, Feb 18, 2021 at 02:27:29PM -0500, Peter Xu wrote:
> 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.

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

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

[Peter Xu]

On Fri, Feb 19, 2021 at 09:16:11AM +0100, Daniel Wagner wrote:
> Hi Peter,

Hi, Daniel,

> 
> On Thu, Feb 18, 2021 at 02:27:29PM -0500, Peter Xu wrote:
> > 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.
> 
> 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.

Thanks,

-- 
Peter Xu

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

[Daniel Wagner]

On Fri, Feb 19, 2021 at 10:24:17AM -0500, Peter Xu wrote:
> 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 :)

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

[John Kacur]

On Fri, 19 Feb 2021, Daniel Wagner wrote:

> Hi Peter,
> 
> On Thu, Feb 18, 2021 at 02:27:29PM -0500, Peter Xu wrote:
> > 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.
> 
> My 2 cents: If parse_cpumask() is to strict fix parse_cpumask() so all

parse_cpumask() correctly examines the current environment and creates
an intersection between the requested cpus and the ones it has permission 
to use. Using taskset with the same parameters as passed to the tools gets 
around that.

I have a few comments

1. Make the programs in the suite work the same way where that makes 
sense, but let's not make a fetish of it, sometimes there are good reasons 
not to impose the conformity.

2. If the user intentionally wants to go outside of the constraints, we 
could use a --force option to make sure the user is aware they are doing 
so.

In private chats with Peter though, he pointed out that if you're always 
using the --force option, then maybe it makes sense for that to be the 
default, and in addition, should calls to setaffinity() be the test?


3. We need to think a little bit about whether and how cyclictest and 
friends should run in containers.

> 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"

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

> 
> Thanks,
> Daniel
> 

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

[Peter Xu]

Daniel,

On Fri, Feb 19, 2021 at 05:20:19PM +0100, Daniel Wagner wrote:
> On Fri, Feb 19, 2021 at 10:24:17AM -0500, Peter Xu wrote:
> > 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 :)

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,

-- 
Peter Xu

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

[Daniel Wagner]

On 19.02.21 17:35, John Kacur wrote:
>> "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?

max_cpus is just giving you the number of available CPUs. The loop 
assumes that all are in the range of [0..max_cpus] which is not 
necessarily true. You need to iterate over all possible CPUs. That is 
why passing in max_cpus is pretty useless.

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

[Daniel Wagner]

Hi Peter,

On 19.02.21 17:39, Peter Xu wrote:
> 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.

> 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.

Thanks,
Daniel

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

[John Kacur]

On Thu, 18 Feb 2021, Peter Xu wrote:

> 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 <jkacur@redhat.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Clark Williams <williams@redhat.com>
> Reported-by: Pradipta Kumar Sahoo <psahoo@redhat.com>
> Reported-by: Mike Stowell <mstowell@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  src/oslat/oslat.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index b2c5373..465a694 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -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);
>  
> -- 
> 2.26.2
> 
> 

Signed-off-by: John Kacur <jkacur@redhat.com>

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

[John Kacur]

On Fri, 19 Feb 2021, Peter Xu wrote:

> Daniel,
> 
> On Fri, Feb 19, 2021 at 05:20:19PM +0100, Daniel Wagner wrote:
> > On Fri, Feb 19, 2021 at 10:24:17AM -0500, Peter Xu wrote:
> > > 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 :)
> 
> 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,
> 
> -- 
> Peter Xu
> 
> 

Hey, I accepted your patch, we're just discussing how to make things work
in the best way for the whole suite in the future.

Thanks for your patches!

John

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

[Peter Xu]

On Fri, Feb 19, 2021 at 05:49:47PM +0100, Daniel Wagner wrote:
> Hi Peter,
> 
> On 19.02.21 17:39, Peter Xu wrote:
> > 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.
> 
> > 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.

It's not really my problem, I can compile my own binary.  It's just that I want
to have it working as usual for anyone using it.  Also it'll be great to have
it fixed sooner for our downstream so our QE won't be affect.  I don't want the
unbreak to be delayed by any further cleanup idea anymore - I am totally not
against that, but again I think they can be done on top of a working tree.

Sorry if my wording was too harsh - I apologize for that.

Thanks,

-- 
Peter Xu