diff mbox series

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

Message ID 20210210165407.9770-3-dwagner@suse.de
State New
Headers show
Series oslat fixes | expand

Commit Message

Daniel Wagner Feb. 10, 2021, 4:54 p.m. UTC
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(-)

Comments

Peter Xu Feb. 10, 2021, 5:14 p.m. UTC | #1
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,
John Kacur Feb. 11, 2021, 4:28 a.m. UTC | #2
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
Peter Xu Feb. 11, 2021, 3:24 p.m. UTC | #3
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
diff mbox series

Patch

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