Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 7 Oct 2021 19:05:58 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Shunsuke Nakamura <nakamura.shun@fujitsu.com>
Cc:     peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
        mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
        namhyung@kernel.org, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org
Subject: Re: [PATCH v2] libperf tests: Fix test_stat_cpu
Message-ID: <YV8o9nukzBTkWmlw@krava>
References: <20211006094817.477494-1-nakamura.shun@fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211006094817.477494-1-nakamura.shun@fujitsu.com>
Precedence: bulk

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
>