lkml.org 
[lkml]   [2021]   [Dec]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 03/22] perf stat: Switch aggregation to use for_each loop
On Tue, Dec 07, 2021 at 06:45:48PM -0800, Ian Rogers wrote:
> Tidy up the use of cpu and index to hopefully make the code less error
> prone. Avoid unused warnings with (void) which will be removed in a
> later patch.
>
> In aggr_update_shadow, the perf_cpu_map is switched from
> the evlist to the counter's cpu map, so the index is appropriate. This
> addresses a problem where uncore counts, with a cpumap like:
> $ cat /sys/devices/uncore_imc_0/cpumask
> 0,18
> Don't aggregate counts in CPUs based on the index of those values in the
> cpumap (0 and 1) but on the actual CPU (0 and 18). Thereby correcting
> metric calculations in per-socket mode for counters with without a full
> cpumask.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/stat-display.c | 48 +++++++++++++++++++---------------
> 1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 588601000f3f..efab39a759ff 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -330,8 +330,8 @@ static void print_metric_header(struct perf_stat_config *config,
> static int first_shadow_cpu(struct perf_stat_config *config,
> struct evsel *evsel, struct aggr_cpu_id id)
> {
> - struct evlist *evlist = evsel->evlist;
> - int i;
> + struct perf_cpu_map *cpus;
> + int cpu, idx;
>
> if (config->aggr_mode == AGGR_NONE)
> return id.core;
> @@ -339,14 +339,11 @@ static int first_shadow_cpu(struct perf_stat_config *config,
> if (!config->aggr_get_id)
> return 0;
>
> - for (i = 0; i < evsel__nr_cpus(evsel); i++) {
> - int cpu2 = evsel__cpus(evsel)->map[i];
> -
> - if (cpu_map__compare_aggr_cpu_id(
> - config->aggr_get_id(config, evlist->core.cpus, cpu2),
> - id)) {
> - return cpu2;
> - }
> + cpus = evsel__cpus(evsel);
> + perf_cpu_map__for_each_cpu(cpu, idx, cpus) {
> + if (cpu_map__compare_aggr_cpu_id(config->aggr_get_id(config, cpus, idx),
> + id))
> + return cpu;

so this looks strange, you pass idx instead of cpu2 to aggr_get_id,
which takes idx as 3rd argument, so it looks like it was broken now,
should this be a separate fix?

also the original code for some reason passed evlist->core.cpus
to aggr_get_id, which might differ rom evsel's cpus

same for aggr_update_shadow change

jirka

\
 
 \ /
  Last update: 2021-12-11 20:25    [W:1.021 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site