linux-kernel - Re: [PATCH v2] libperf tests: Fix test_stat

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <OSBPR01MB4600710A07177677DF2F6E0CF7B59@OSBPR01MB4600.jpnprd01.prod.outlook.com>
Date:   Mon, 11 Oct 2021 08:11:49 +0000
From:   "nakamura.shun@...itsu.com" <nakamura.shun@...itsu.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "alexander.shishkin@...ux.intel.com" 
        <alexander.shishkin@...ux.intel.com>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>
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 <nakamura.shun@...itsu.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