[PATCH rt-tests 0/2] oslat fixes

[PATCH rt-tests 0/2] oslat fixes

[Daniel Wagner]

A couple of fixes for the bugs I introduced recently, reported by Peter.

Cc: Peter Xu <peterx@redhat.com>

Daniel Wagner (2):
  oslat: Print version string
  oslat: Use cpuset size as upper bound

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

-- 
2.30.0

[PATCH rt-tests 1/2] oslat: Print version string

[Daniel Wagner]

During the streamlining of the command line options we missed to hook
up the command line option '--version' with usage().

Fixes: e411219d27b1 ("oslat: Streamline usage output and man page")
Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/oslat/oslat.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index 5b7e0d5b5d5c..d3f659b218b0 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -655,16 +655,10 @@ static void parse_options(int argc, char *argv[])
 			 */
 			g.single_preheat_thread = true;
 			break;
-		case 'v':
-			/*
-			 * Because we always dump the version even before parsing options,
-			 * what we need to do is to quit..
-			 */
-			exit(0);
-			break;
 		case 'z':
 			g.output_omit_zero_buckets = 1;
 			break;
+		case 'v':
 		case 'h':
 			usage(0);
 			break;
-- 
2.30.0

Re: [PATCH rt-tests 1/2] oslat: Print version string

[Peter Xu]

Hi, Daniel,

On Wed, Feb 10, 2021 at 05:54:06PM +0100, Daniel Wagner wrote:
> During the streamlining of the command line options we missed to hook
> up the command line option '--version' with usage().
> 
> Fixes: e411219d27b1 ("oslat: Streamline usage output and man page")
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/oslat/oslat.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 5b7e0d5b5d5c..d3f659b218b0 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -655,16 +655,10 @@ static void parse_options(int argc, char *argv[])
>  			 */
>  			g.single_preheat_thread = true;
>  			break;
> -		case 'v':
> -			/*
> -			 * Because we always dump the version even before parsing options,
> -			 * what we need to do is to quit..
> -			 */
> -			exit(0);
> -			break;
>  		case 'z':
>  			g.output_omit_zero_buckets = 1;
>  			break;
> +		case 'v':
>  		case 'h':
>  			usage(0);
>  			break;
> -- 
> 2.30.0

Thanks for your quick response.  I've commented here:

https://lore.kernel.org/linux-rt-users/20210210170015.GL103365@xz-x1/

-- 
Peter Xu

Re: [PATCH rt-tests 1/2] oslat: Print version string

[John Kacur]

On Wed, 10 Feb 2021, Daniel Wagner wrote:

> During the streamlining of the command line options we missed to hook
> up the command line option '--version' with usage().
> 
> Fixes: e411219d27b1 ("oslat: Streamline usage output and man page")
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/oslat/oslat.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 5b7e0d5b5d5c..d3f659b218b0 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -655,16 +655,10 @@ static void parse_options(int argc, char *argv[])
>  			 */
>  			g.single_preheat_thread = true;
>  			break;
> -		case 'v':
> -			/*
> -			 * Because we always dump the version even before parsing options,
> -			 * what we need to do is to quit..
> -			 */
> -			exit(0);
> -			break;
>  		case 'z':
>  			g.output_omit_zero_buckets = 1;
>  			break;
> +		case 'v':
>  		case 'h':
>  			usage(0);
>  			break;
> -- 
> 2.30.0
> 
> 

It shouldn't be a fall through, we don't want to print the usage.

Re: [PATCH rt-tests 1/2] oslat: Print version string

[Daniel Wagner]

On Wed, Feb 10, 2021 at 12:28:09PM -0500, John Kacur wrote:
> It shouldn't be a fall through, we don't want to print the usage.

Ok. In this case I suggest we introduce for all tolls the long option
'--version' and print only the version string.

As I have already added the long parsing option in my series 'Generate
machine-readable output'. This was a lot of tedious work, so I would
like to avoid to do it split out for the '--version' option. Thus, could
you review the series and I could easily fix this problem.

Re: [PATCH rt-tests 1/2] oslat: Print version string

[Peter Xu]

On Wed, Feb 10, 2021 at 06:33:53PM +0100, Daniel Wagner wrote:
> On Wed, Feb 10, 2021 at 12:28:09PM -0500, John Kacur wrote:
> > It shouldn't be a fall through, we don't want to print the usage.
> 
> Ok. In this case I suggest we introduce for all tolls the long option
> '--version' and print only the version string.
> 
> As I have already added the long parsing option in my series 'Generate
> machine-readable output'. This was a lot of tedious work, so I would
> like to avoid to do it split out for the '--version' option. Thus, could
> you review the series and I could easily fix this problem.

It's great idea to have JSON format for all the tools.  However I just don't
see why "-v"/"--version" is related to your machine readable output work.
Could you exlaborate?  Thanks,

-- 
Peter Xu

Re: [PATCH rt-tests 1/2] oslat: Print version string

[Daniel Wagner]

> It's great idea to have JSON format for all the tools.  However I just don't
> see why "-v"/"--version" is related to your machine readable output work.
> Could you exlaborate?  Thanks,

I've added long options support to many rt-tests in the mentioned
series. I would like to base this fix on top of this work.

Re: [PATCH rt-tests 1/2] oslat: Print version string

[Peter Xu]

On Wed, Feb 10, 2021 at 07:15:04PM +0100, Daniel Wagner wrote:
> > It's great idea to have JSON format for all the tools.  However I just don't
> > see why "-v"/"--version" is related to your machine readable output work.
> > Could you exlaborate?  Thanks,
> 
> I've added long options support to many rt-tests in the mentioned
> series. I would like to base this fix on top of this work.

My opinion probably don't even matter here... but I tend to agree with John
that the fix and your work is separated.  If there's must a rebase, it would be
better to rebase the big series upon the fix rather than the other way round,
simply because a fix is normally more important than a new feature (if not
talking about this "-v" thing, the other --cpu-list issue should be important).

Appreciate all the refactoring works on oslat happened and ongoing, it would
just be even nicer if the touched code path can be smoked so we don't miss
anything important.

Thanks again.

-- 
Peter Xu

Re: [PATCH rt-tests 1/2] oslat: Print version string

[John Kacur]

On Wed, 10 Feb 2021, Daniel Wagner wrote:

> On Wed, Feb 10, 2021 at 12:28:09PM -0500, John Kacur wrote:
> > It shouldn't be a fall through, we don't want to print the usage.
> 
> Ok. In this case I suggest we introduce for all tolls the long option
> '--version' and print only the version string.
> 
> As I have already added the long parsing option in my series 'Generate
> machine-readable output'. This was a lot of tedious work, so I would
> like to avoid to do it split out for the '--version' option. Thus, could
> you review the series and I could easily fix this problem.
> 

Are you really telling me you won't fix a problem you introduced unless
I first take all the other code you would like to get in rt-tests?

:)


John

Re: [PATCH rt-tests 1/2] oslat: Print version string

[Daniel Wagner]

On Wed, Feb 10, 2021 at 12:59:29PM -0500, John Kacur wrote:
> Are you really telling me you won't fix a problem you introduced unless
> I first take all the other code you would like to get in rt-tests?

I've already done the long option support in the JSON series. It would
make my life way simpler if those bits would be in the tree.

I think, the simplest fix for now is to temporarily a local fix for
oslat.

Re: [PATCH rt-tests 1/2] oslat: Print version string

[John Kacur]

On Wed, 10 Feb 2021, Daniel Wagner wrote:

> On Wed, Feb 10, 2021 at 12:59:29PM -0500, John Kacur wrote:
> > Are you really telling me you won't fix a problem you introduced unless
> > I first take all the other code you would like to get in rt-tests?
> 
> I've already done the long option support in the JSON series. It would
> make my life way simpler if those bits would be in the tree.
> 
> I think, the simplest fix for now is to temporarily a local fix for
> oslat.
> 

Just add a fix onto for the oslat problem on the end of your series
I'll be done the review soon.

If we need to do any backporting then we'll do that.

Cheers, Thanks

John

Re: [PATCH rt-tests 1/2] oslat: Print version string

[John Kacur]

On Wed, 10 Feb 2021, John Kacur wrote:

> 
> 
> On Wed, 10 Feb 2021, Daniel Wagner wrote:
> 
> > On Wed, Feb 10, 2021 at 12:59:29PM -0500, John Kacur wrote:
> > > Are you really telling me you won't fix a problem you introduced unless
> > > I first take all the other code you would like to get in rt-tests?
> > 
> > I've already done the long option support in the JSON series. It would
> > make my life way simpler if those bits would be in the tree.
> > 
> > I think, the simplest fix for now is to temporarily a local fix for
> > oslat.
> > 
> 
> Just add a fix onto for the oslat problem on the end of your series
> I'll be done the review soon.
> 
> If we need to do any backporting then we'll do that.
> 
> Cheers, Thanks
> 
> John
> 


I rewound your patches in git, and applied the two oslat fixes and
reapplied your patches with a git interactive rebase, and then I pushed
them for others to try before I'm ready to push them upstream

Anyone who wants to try the new json stuff it's in a branch called

unstable/devel/rt-numa-json

The numa changes need more testing before integrate them, but 
this should make it easier for others to try it out.

John

[PATCH rt-tests 2/2] oslat: Use cpuset size as upper bound

[Daniel Wagner]

To assign the threads to the correct CPU we need to use the cpuset
size as upper bound for the loop and not the number of threads.

Fixes: 85b0763dacd9 ("oslat: Use parse_cpumask() from rt-numa.h")
Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/oslat/oslat.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index d3f659b218b0..62b82098419a 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -742,9 +742,12 @@ int main(int argc, char *argv[])
 	n_cores = numa_bitmask_weight(cpu_set);
 
 	TEST(threads = calloc(1, n_cores * sizeof(threads[0])));
-	for (i = 0; i < n_cores; ++i)
-		if (numa_bitmask_isbitset(cpu_set, i) && move_to_core(i) == 0)
+	for (i = 0; n_cores && i < cpu_set->size; i++) {
+		if (numa_bitmask_isbitset(cpu_set, i) && move_to_core(i) == 0) {
 			threads[g.n_threads_total++].core_i = i;
+			n_cores--;
+		}
+	}
 
 	if (numa_bitmask_isbitset(cpu_set, 0) && g.rtprio)
 		printf("WARNING: Running SCHED_FIFO workload on CPU 0 may hang the thread\n");
-- 
2.30.0

Re: [PATCH rt-tests 2/2] oslat: Use cpuset size as upper bound

[Peter Xu]

On Wed, Feb 10, 2021 at 05:54:07PM +0100, Daniel Wagner wrote:
> To assign the threads to the correct CPU we need to use the cpuset
> size as upper bound for the loop and not the number of threads.
> 
> Fixes: 85b0763dacd9 ("oslat: Use parse_cpumask() from rt-numa.h")
> Reported-by: Peter Xu <peterx@redhat.com>

Please instead add:

Reported-by: Pradipta Kumar Sahoo <psahoo@redhat.com>

Pradipta, would you verify and see whether this patch fixes your issue?

Thanks,

-- 
Peter Xu

Re: [PATCH rt-tests 2/2] oslat: Use cpuset size as upper bound

[John Kacur]

On Wed, 10 Feb 2021, Daniel Wagner wrote:

> To assign the threads to the correct CPU we need to use the cpuset
> size as upper bound for the loop and not the number of threads.
> 
> Fixes: 85b0763dacd9 ("oslat: Use parse_cpumask() from rt-numa.h")
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/oslat/oslat.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index d3f659b218b0..62b82098419a 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -742,9 +742,12 @@ int main(int argc, char *argv[])
>  	n_cores = numa_bitmask_weight(cpu_set);
>  
>  	TEST(threads = calloc(1, n_cores * sizeof(threads[0])));
> -	for (i = 0; i < n_cores; ++i)
> -		if (numa_bitmask_isbitset(cpu_set, i) && move_to_core(i) == 0)
> +	for (i = 0; n_cores && i < cpu_set->size; i++) {
> +		if (numa_bitmask_isbitset(cpu_set, i) && move_to_core(i) == 0) {
>  			threads[g.n_threads_total++].core_i = i;
> +			n_cores--;
> +		}
> +	}
>  
>  	if (numa_bitmask_isbitset(cpu_set, 0) && g.rtprio)
>  		printf("WARNING: Running SCHED_FIFO workload on CPU 0 may hang the thread\n");
> -- 
> 2.30.0
> 
> 

This looks okay, but we are waiting for Pradipta's testing to confirm 
before I push it.

Thanks

John

Re: [PATCH rt-tests 2/2] oslat: Use cpuset size as upper bound

[Peter Xu]

On Wed, Feb 10, 2021 at 11:28:16PM -0500, John Kacur wrote:
> 
> 
> On Wed, 10 Feb 2021, Daniel Wagner wrote:
> 
> > To assign the threads to the correct CPU we need to use the cpuset
> > size as upper bound for the loop and not the number of threads.
> > 
> > Fixes: 85b0763dacd9 ("oslat: Use parse_cpumask() from rt-numa.h")
> > Reported-by: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Daniel Wagner <dwagner@suse.de>
> > ---
> >  src/oslat/oslat.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> > index d3f659b218b0..62b82098419a 100644
> > --- a/src/oslat/oslat.c
> > +++ b/src/oslat/oslat.c
> > @@ -742,9 +742,12 @@ int main(int argc, char *argv[])
> >  	n_cores = numa_bitmask_weight(cpu_set);
> >  
> >  	TEST(threads = calloc(1, n_cores * sizeof(threads[0])));
> > -	for (i = 0; i < n_cores; ++i)
> > -		if (numa_bitmask_isbitset(cpu_set, i) && move_to_core(i) == 0)
> > +	for (i = 0; n_cores && i < cpu_set->size; i++) {
> > +		if (numa_bitmask_isbitset(cpu_set, i) && move_to_core(i) == 0) {
> >  			threads[g.n_threads_total++].core_i = i;
> > +			n_cores--;
> > +		}
> > +	}
> >  
> >  	if (numa_bitmask_isbitset(cpu_set, 0) && g.rtprio)
> >  		printf("WARNING: Running SCHED_FIFO workload on CPU 0 may hang the thread\n");
> > -- 
> > 2.30.0
> > 
> > 
> 
> This looks okay, but we are waiting for Pradipta's testing to confirm 
> before I push it.

After the reporter's confirmation there seems to have two issues.  Daniel's
patch should be a valid fix of at least one of the problem.  The other problem
is still under investigation and I think it's not affecting the upstream
branch.  So I think we don't need to wait for that anymore:

Reviewed-by: Peter Xu <peterx@redhat.com>
Tested-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu