All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] libperf tests: Fix test_stat_cpu
@ 2021-10-06  9:48 Shunsuke Nakamura
  2021-10-07 17:05 ` Jiri Olsa
  0 siblings, 1 reply; 4+ messages in thread
From: Shunsuke Nakamura @ 2021-10-06  9:48 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung
  Cc: linux-kernel, linux-perf-users

`cpu` of perf_evsel__read() must be specified the cpu index.
perf_cpu_map__for_each_cpu is for iterating the cpu number (not index)
and is not appropriate.
So, if there is an offline CPU, the cpu number specified in the argument
may point out of range because the cpu number and the cpu index are
different.

Fix test_stat_cpu.

Committer testing:

  # make tests -C tools/lib/perf/
  make: Entering directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'
  running static:
  - running tests/test-cpumap.c...OK
  - running tests/test-threadmap.c...OK
  - running tests/test-evlist.c...OK
  - running tests/test-evsel.c...OK
  running dynamic:
  - running tests/test-cpumap.c...OK
  - running tests/test-threadmap.c...OK
  - running tests/test-evlist.c...OK
  - running tests/test-evsel.c...OK
  make: Leaving directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'


Signed-off-by: Shunsuke Nakamura <nakamura.shun@fujitsu.com>
---
Previous version at:
https://lore.kernel.org/lkml/20211006080456.474273-1-nakamura.shun@fujitsu.com/

Changes in v2:
 - Remove "2/2" from Patch Subject

 tools/lib/perf/tests/test-evlist.c | 6 +++---
 tools/lib/perf/tests/test-evsel.c  | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
index c67c83399170..47badd7eabf2 100644
--- a/tools/lib/perf/tests/test-evlist.c
+++ b/tools/lib/perf/tests/test-evlist.c
@@ -40,7 +40,7 @@ static int test_stat_cpu(void)
 		.type	= PERF_TYPE_SOFTWARE,
 		.config	= PERF_COUNT_SW_TASK_CLOCK,
 	};
-	int err, cpu, tmp;
+	int err, idx;
 
 	cpus = perf_cpu_map__new(NULL);
 	__T("failed to create cpus", cpus);
@@ -70,10 +70,10 @@ static int test_stat_cpu(void)
 	perf_evlist__for_each_evsel(evlist, evsel) {
 		cpus = perf_evsel__cpus(evsel);
 
-		perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
+		for (idx = 0, idx < perf_cpu_map__nr(cpus); idx++) {
 			struct perf_counts_values counts = { .val = 0 };
 
-			perf_evsel__read(evsel, cpu, 0, &counts);
+			perf_evsel__read(evsel, idx, 0, &counts);
 			__T("failed to read value for evsel", counts.val != 0);
 		}
 	}
diff --git a/tools/lib/perf/tests/test-evsel.c b/tools/lib/perf/tests/test-evsel.c
index 9abd4c0bf6db..33ae9334861a 100644
--- a/tools/lib/perf/tests/test-evsel.c
+++ b/tools/lib/perf/tests/test-evsel.c
@@ -22,7 +22,7 @@ static int test_stat_cpu(void)
 		.type	= PERF_TYPE_SOFTWARE,
 		.config	= PERF_COUNT_SW_CPU_CLOCK,
 	};
-	int err, cpu, tmp;
+	int err, idx;
 
 	cpus = perf_cpu_map__new(NULL);
 	__T("failed to create cpus", cpus);
@@ -33,10 +33,10 @@ static int test_stat_cpu(void)
 	err = perf_evsel__open(evsel, cpus, NULL);
 	__T("failed to open evsel", err == 0);
 
-	perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
+	for (idx = 0; idx < perf_cpu_map__nr(cpus); idx++) {
 		struct perf_counts_values counts = { .val = 0 };
 
-		perf_evsel__read(evsel, cpu, 0, &counts);
+		perf_evsel__read(evsel, idx, 0, &counts);
 		__T("failed to read value for evsel", counts.val != 0);
 	}
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] libperf tests: Fix test_stat_cpu
  2021-10-06  9:48 [PATCH v2] libperf tests: Fix test_stat_cpu Shunsuke Nakamura
@ 2021-10-07 17:05 ` Jiri Olsa
  2021-10-08 18:53   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2021-10-07 17:05 UTC (permalink / raw)
  To: Shunsuke Nakamura
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, namhyung,
	linux-kernel, linux-perf-users

On Wed, Oct 06, 2021 at 06:48:17PM +0900, Shunsuke Nakamura wrote:
> `cpu` of perf_evsel__read() must be specified the cpu index.
> perf_cpu_map__for_each_cpu is for iterating the cpu number (not index)
> and is not appropriate.
> So, if there is an offline CPU, the cpu number specified in the argument
> may point out of range because the cpu number and the cpu index are
> different.

nice catch

> 
> Fix test_stat_cpu.
> 
> Committer testing:
> 
>   # make tests -C tools/lib/perf/
>   make: Entering directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'
>   running static:
>   - running tests/test-cpumap.c...OK
>   - running tests/test-threadmap.c...OK
>   - running tests/test-evlist.c...OK
>   - running tests/test-evsel.c...OK
>   running dynamic:
>   - running tests/test-cpumap.c...OK
>   - running tests/test-threadmap.c...OK
>   - running tests/test-evlist.c...OK
>   - running tests/test-evsel.c...OK
>   make: Leaving directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'
> 
> 
> Signed-off-by: Shunsuke Nakamura <nakamura.shun@fujitsu.com>
> ---
> Previous version at:
> https://lore.kernel.org/lkml/20211006080456.474273-1-nakamura.shun@fujitsu.com/
> 
> Changes in v2:
>  - Remove "2/2" from Patch Subject
> 
>  tools/lib/perf/tests/test-evlist.c | 6 +++---
>  tools/lib/perf/tests/test-evsel.c  | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
> index c67c83399170..47badd7eabf2 100644
> --- a/tools/lib/perf/tests/test-evlist.c
> +++ b/tools/lib/perf/tests/test-evlist.c
> @@ -40,7 +40,7 @@ static int test_stat_cpu(void)
>  		.type	= PERF_TYPE_SOFTWARE,
>  		.config	= PERF_COUNT_SW_TASK_CLOCK,
>  	};
> -	int err, cpu, tmp;
> +	int err, idx;
>  
>  	cpus = perf_cpu_map__new(NULL);
>  	__T("failed to create cpus", cpus);
> @@ -70,10 +70,10 @@ static int test_stat_cpu(void)
>  	perf_evlist__for_each_evsel(evlist, evsel) {
>  		cpus = perf_evsel__cpus(evsel);
>  
> -		perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
> +		for (idx = 0, idx < perf_cpu_map__nr(cpus); idx++) {

s/,/;/                      ^

tests/test-evlist.c: In function ‘test_stat_cpu’:
tests/test-evlist.c:73:52: error: expected ‘;’ before ‘)’ token
   73 |   for (idx = 0, idx < perf_cpu_map__nr(cpus); idx++) {
      |                                                    ^
      |                                                    ;


perf_cpu_map__for_each_cpu also returns the cpu index (tmp),
maybe we could use that instead?

thanks,
jirka

>  			struct perf_counts_values counts = { .val = 0 };
>  
> -			perf_evsel__read(evsel, cpu, 0, &counts);
> +			perf_evsel__read(evsel, idx, 0, &counts);
>  			__T("failed to read value for evsel", counts.val != 0);
>  		}
>  	}
> diff --git a/tools/lib/perf/tests/test-evsel.c b/tools/lib/perf/tests/test-evsel.c
> index 9abd4c0bf6db..33ae9334861a 100644
> --- a/tools/lib/perf/tests/test-evsel.c
> +++ b/tools/lib/perf/tests/test-evsel.c
> @@ -22,7 +22,7 @@ static int test_stat_cpu(void)
>  		.type	= PERF_TYPE_SOFTWARE,
>  		.config	= PERF_COUNT_SW_CPU_CLOCK,
>  	};
> -	int err, cpu, tmp;
> +	int err, idx;
>  
>  	cpus = perf_cpu_map__new(NULL);
>  	__T("failed to create cpus", cpus);
> @@ -33,10 +33,10 @@ static int test_stat_cpu(void)
>  	err = perf_evsel__open(evsel, cpus, NULL);
>  	__T("failed to open evsel", err == 0);
>  
> -	perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
> +	for (idx = 0; idx < perf_cpu_map__nr(cpus); idx++) {
>  		struct perf_counts_values counts = { .val = 0 };
>  
> -		perf_evsel__read(evsel, cpu, 0, &counts);
> +		perf_evsel__read(evsel, idx, 0, &counts);
>  		__T("failed to read value for evsel", counts.val != 0);
>  	}
>  
> -- 
> 2.25.1
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] libperf tests: Fix test_stat_cpu
  2021-10-07 17:05 ` Jiri Olsa
@ 2021-10-08 18:53   ` Arnaldo Carvalho de Melo
  2021-10-11  8:11     ` nakamura.shun
  0 siblings, 1 reply; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-10-08 18:53 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Shunsuke Nakamura, peterz, mingo, mark.rutland,
	alexander.shishkin, namhyung, linux-kernel, linux-perf-users

Em Thu, Oct 07, 2021 at 07:05:58PM +0200, Jiri Olsa escreveu:
> On Wed, Oct 06, 2021 at 06:48:17PM +0900, Shunsuke Nakamura wrote:
> > `cpu` of perf_evsel__read() must be specified the cpu index.
> > perf_cpu_map__for_each_cpu is for iterating the cpu number (not index)
> > and is not appropriate.
> > So, if there is an offline CPU, the cpu number specified in the argument
> > may point out of range because the cpu number and the cpu index are
> > different.
> 
> nice catch

Indeed, Nakamura-san, please address Jiri's comment and resubmit,

Thanks,

- Arnaldo
 
> > 
> > Fix test_stat_cpu.
> > 
> > Committer testing:
> > 
> >   # make tests -C tools/lib/perf/
> >   make: Entering directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'
> >   running static:
> >   - running tests/test-cpumap.c...OK
> >   - running tests/test-threadmap.c...OK
> >   - running tests/test-evlist.c...OK
> >   - running tests/test-evsel.c...OK
> >   running dynamic:
> >   - running tests/test-cpumap.c...OK
> >   - running tests/test-threadmap.c...OK
> >   - running tests/test-evlist.c...OK
> >   - running tests/test-evsel.c...OK
> >   make: Leaving directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'
> > 
> > 
> > Signed-off-by: Shunsuke Nakamura <nakamura.shun@fujitsu.com>
> > ---
> > Previous version at:
> > https://lore.kernel.org/lkml/20211006080456.474273-1-nakamura.shun@fujitsu.com/
> > 
> > Changes in v2:
> >  - Remove "2/2" from Patch Subject
> > 
> >  tools/lib/perf/tests/test-evlist.c | 6 +++---
> >  tools/lib/perf/tests/test-evsel.c  | 6 +++---
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
> > index c67c83399170..47badd7eabf2 100644
> > --- a/tools/lib/perf/tests/test-evlist.c
> > +++ b/tools/lib/perf/tests/test-evlist.c
> > @@ -40,7 +40,7 @@ static int test_stat_cpu(void)
> >  		.type	= PERF_TYPE_SOFTWARE,
> >  		.config	= PERF_COUNT_SW_TASK_CLOCK,
> >  	};
> > -	int err, cpu, tmp;
> > +	int err, idx;
> >  
> >  	cpus = perf_cpu_map__new(NULL);
> >  	__T("failed to create cpus", cpus);
> > @@ -70,10 +70,10 @@ static int test_stat_cpu(void)
> >  	perf_evlist__for_each_evsel(evlist, evsel) {
> >  		cpus = perf_evsel__cpus(evsel);
> >  
> > -		perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
> > +		for (idx = 0, idx < perf_cpu_map__nr(cpus); idx++) {
> 
> s/,/;/                      ^
> 
> tests/test-evlist.c: In function ‘test_stat_cpu’:
> tests/test-evlist.c:73:52: error: expected ‘;’ before ‘)’ token
>    73 |   for (idx = 0, idx < perf_cpu_map__nr(cpus); idx++) {
>       |                                                    ^
>       |                                                    ;
> 
> 
> perf_cpu_map__for_each_cpu also returns the cpu index (tmp),
> maybe we could use that instead?
> 
> thanks,
> jirka
> 
> >  			struct perf_counts_values counts = { .val = 0 };
> >  
> > -			perf_evsel__read(evsel, cpu, 0, &counts);
> > +			perf_evsel__read(evsel, idx, 0, &counts);
> >  			__T("failed to read value for evsel", counts.val != 0);
> >  		}
> >  	}
> > diff --git a/tools/lib/perf/tests/test-evsel.c b/tools/lib/perf/tests/test-evsel.c
> > index 9abd4c0bf6db..33ae9334861a 100644
> > --- a/tools/lib/perf/tests/test-evsel.c
> > +++ b/tools/lib/perf/tests/test-evsel.c
> > @@ -22,7 +22,7 @@ static int test_stat_cpu(void)
> >  		.type	= PERF_TYPE_SOFTWARE,
> >  		.config	= PERF_COUNT_SW_CPU_CLOCK,
> >  	};
> > -	int err, cpu, tmp;
> > +	int err, idx;
> >  
> >  	cpus = perf_cpu_map__new(NULL);
> >  	__T("failed to create cpus", cpus);
> > @@ -33,10 +33,10 @@ static int test_stat_cpu(void)
> >  	err = perf_evsel__open(evsel, cpus, NULL);
> >  	__T("failed to open evsel", err == 0);
> >  
> > -	perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
> > +	for (idx = 0; idx < perf_cpu_map__nr(cpus); idx++) {
> >  		struct perf_counts_values counts = { .val = 0 };
> >  
> > -		perf_evsel__read(evsel, cpu, 0, &counts);
> > +		perf_evsel__read(evsel, idx, 0, &counts);
> >  		__T("failed to read value for evsel", counts.val != 0);
> >  	}
> >  
> > -- 
> > 2.25.1
> > 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] libperf tests: Fix test_stat_cpu
  2021-10-08 18:53   ` Arnaldo Carvalho de Melo
@ 2021-10-11  8:11     ` nakamura.shun
  0 siblings, 0 replies; 4+ messages in thread
From: nakamura.shun @ 2021-10-11  8:11 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: peterz, mingo, mark.rutland, alexander.shishkin, namhyung,
	linux-kernel, linux-perf-users

Hi Arnaldo, jirka

> Em Thu, Oct 07, 2021 at 07:05:58PM +0200, Jiri Olsa escreveu:
> > On Wed, Oct 06, 2021 at 06:48:17PM +0900, Shunsuke Nakamura wrote:
> > > `cpu` of perf_evsel__read() must be specified the cpu index.
> > > perf_cpu_map__for_each_cpu is for iterating the cpu number (not index)
> > > and is not appropriate.
> > > So, if there is an offline CPU, the cpu number specified in the argument
> > > may point out of range because the cpu number and the cpu index are
> > > different.
> > 
> > nice catch
> 
> Indeed, Nakamura-san, please address Jiri's comment and resubmit,

I'll send it right away.


> 
> Thanks,
> 
> - Arnaldo
>  
> > > 
> > > Fix test_stat_cpu.
> > > 
> > > Committer testing:
> > > 
> > >   # make tests -C tools/lib/perf/
> > >   make: Entering directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'
> > >   running static:
> > >   - running tests/test-cpumap.c...OK
> > >   - running tests/test-threadmap.c...OK
> > >   - running tests/test-evlist.c...OK
> > >   - running tests/test-evsel.c...OK
> > >   running dynamic:
> > >   - running tests/test-cpumap.c...OK
> > >   - running tests/test-threadmap.c...OK
> > >   - running tests/test-evlist.c...OK
> > >   - running tests/test-evsel.c...OK
> > >   make: Leaving directory '/home/nakamura/kernel_src/linux-5.15-rc4_fix/tools/lib/perf'
> > > 
> > > 
> > > Signed-off-by: Shunsuke Nakamura <nakamura.shun@fujitsu.com>
> > > ---
> > > Previous version at:
> > > https://lore.kernel.org/lkml/20211006080456.474273-1-nakamura.shun@fujitsu.com/
> > > 
> > > Changes in v2:
> > >  - Remove "2/2" from Patch Subject
> > > 
> > >  tools/lib/perf/tests/test-evlist.c | 6 +++---
> > >  tools/lib/perf/tests/test-evsel.c  | 6 +++---
> > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
> > > index c67c83399170..47badd7eabf2 100644
> > > --- a/tools/lib/perf/tests/test-evlist.c
> > > +++ b/tools/lib/perf/tests/test-evlist.c
> > > @@ -40,7 +40,7 @@ static int test_stat_cpu(void)
> > >  		.type	= PERF_TYPE_SOFTWARE,
> > >  		.config	= PERF_COUNT_SW_TASK_CLOCK,
> > >  	};
> > > -	int err, cpu, tmp;
> > > +	int err, idx;
> > >  
> > >  	cpus = perf_cpu_map__new(NULL);
> > >  	__T("failed to create cpus", cpus);
> > > @@ -70,10 +70,10 @@ static int test_stat_cpu(void)
> > >  	perf_evlist__for_each_evsel(evlist, evsel) {
> > >  		cpus = perf_evsel__cpus(evsel);
> > >  
> > > -		perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
> > > +		for (idx = 0, idx < perf_cpu_map__nr(cpus); idx++) {
> > 
> > s/,/;/                      ^
> > 
> > tests/test-evlist.c: In function 'test_stat_cpu':
> > tests/test-evlist.c:73:52: error: expected ';' before ')' token
> >    73 |   for (idx = 0, idx < perf_cpu_map__nr(cpus); idx++) {
> >       |                                                    ^
> >       |                                                    ;

Thanks for review.
I will fix it.


> > 
> > 
> > perf_cpu_map__for_each_cpu also returns the cpu index (tmp),
> > maybe we could use that instead?

Using only cpu index results in a build error.


  $ git diff
  diff --git a/tools/lib/perf/tests/test-evlist.c b/tools/lib/perf/tests/test-evlist.c
  index c67c83399170..b36aa8afe95c 100644
  --- a/tools/lib/perf/tests/test-evlist.c
  +++ b/tools/lib/perf/tests/test-evlist.c
  @@ -73,7 +73,7 @@ static int test_stat_cpu(void)
                  perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
                          struct perf_counts_values counts = { .val = 0 };

  -                       perf_evsel__read(evsel, cpu, 0, &counts);
  +                       perf_evsel__read(evsel, tmp, 0, &counts);
                          __T("failed to read value for evsel", counts.val != 0);
                  }
          }


  $ make tests -C tools/lib/perf/
  make: Entering directory '/ubinux-dev/common/home/nakamura/kernel_Build/linux/tools/lib/perf'
    CC      tests/test-evlist.o
  tests/test-evlist.c: In function 'test_stat_cpu':
  tests/test-evlist.c:43:11: error: variable 'cpu' set but not used [-Werror=unused-but-set-variable]
     43 |  int err, cpu, tmp;
        |


Best Regards
Shunsuke

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-11  8:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-06  9:48 [PATCH v2] libperf tests: Fix test_stat_cpu Shunsuke Nakamura
2021-10-07 17:05 ` Jiri Olsa
2021-10-08 18:53   ` Arnaldo Carvalho de Melo
2021-10-11  8:11     ` nakamura.shun

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.