2021-10-06 09:50:59

by [email protected]

Subject: [PATCH v2] libperf tests: Fix test_stat_cpu

`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 <[email protected]>
---
Previous version at:
https://lore.kernel.org/lkml/[email protected]/

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

2021-10-07 19:42:18

by Jiri Olsa

Subject: Re: [PATCH v2] libperf tests: Fix test_stat_cpu

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 <[email protected]>
> ---
> Previous version at:
> https://lore.kernel.org/lkml/[email protected]/
>
> 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
>

2021-10-08 18:56:14

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH v2] libperf tests: Fix test_stat_cpu

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 <[email protected]>
> > ---
> > Previous version at:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > 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

2021-10-11 12:11:30

by [email protected]

Subject: Re: [PATCH v2] libperf tests: Fix test_stat_cpu

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 <[email protected]>
> > > ---
> > > Previous version at:
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > 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