linux-kernel - Re: [PATCH] perf stat: Fix per socket shadow aggregation

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Dec 2021 10:44:36 +0000
From:   James Clark <james.clark@....com>
To:     Ian Rogers <irogers@...gle.com>, Andi Kleen <ak@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        John Garry <john.garry@...wei.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        "Paul A . Clarke" <pc@...ibm.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Riccardo Mancini <rickyman7@...il.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vineet Singh <vineet.singh@...el.com>
Cc:     eranian@...gle.com
Subject: Re: [PATCH] perf stat: Fix per socket shadow aggregation



On 04/12/2021 02:34, Ian Rogers wrote:
> An uncore device may have a CPU mask that specifies one CPU per socket:
> $ cat /sys/devices/uncore_imc_0/cpumask
> 0,18
> The perf_stat_config aggr_map will map a CPU to the socket and other
> aggregation values for it. Fix an error where the index into CPU mask
> was being used as the index into the aggr_map. For the cpumask above the
> indexes 0 and 1 are passed to aggr_map rather than the CPUs 0 and 18.
> 
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
>  tools/perf/util/stat-display.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 588601000f3f..7cfad5cfec38 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -516,7 +516,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>  static void aggr_update_shadow(struct perf_stat_config *config,
>  			       struct evlist *evlist)
>  {
> -	int cpu, s;
> +	int idx, cpu, s;
>  	struct aggr_cpu_id s2, id;
>  	u64 val;
>  	struct evsel *counter;
> @@ -525,11 +525,12 @@ static void aggr_update_shadow(struct perf_stat_config *config,
>  		id = config->aggr_map->map[s];
>  		evlist__for_each_entry(evlist, counter) {
>  			val = 0;
> -			for (cpu = 0; cpu < evsel__nr_cpus(counter); cpu++) {
> +			for (idx = 0; idx < evsel__nr_cpus(counter); idx++) {
> +				cpu = perf_cpu_map__cpu(evsel__cpus(counter), idx);
>  				s2 = config->aggr_get_id(config, evlist->core.cpus, cpu);

Hi Ian,

This same pattern of looping over the CPUs and calling aggr_get_id() is used a couple of
other times. For example in aggr_cb() and first_shadow_cpu(). Do you think these also
need updating?

Or could we fix it in the aggr_get_id() functions so that they expect an index instead
of CPU ID and do the conversion themselves? The callbacks do say "idx" rather than "cpu"
so maybe there is still come confusion.

For example: 

	perf_stat__get_die_cached(struct perf_stat_config *config,
					struct perf_cpu_map *map, int idx)

>  				if (!cpu_map__compare_aggr_cpu_id(s2, id))
>  					continue;
> -				val += perf_counts(counter->counts, cpu, 0)->val;
> +				val += perf_counts(counter->counts, idx, 0)->val;
>  			}
>  			perf_stat__update_shadow_stats(counter, val,
>  					first_shadow_cpu(config, counter, id),
>