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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fVn-uKs+_647D+O6r=n9N98iK=Z3U53WYkPm9NxMEz3gg@mail.gmail.com>
Date:   Mon, 6 Dec 2021 14:16:30 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     James Clark <james.clark@....com>
Cc:     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>, eranian@...gle.com
Subject: Re: [PATCH] perf stat: Fix per socket shadow aggregation

On Mon, Dec 6, 2021 at 2:44 AM James Clark <james.clark@....com> wrote:
>
>
>
> 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?

Thanks for the feedback James!

For first_shadow_cpu the index is translated to the the cpu via the
cpu map here:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core#n343
so I think it is sound.

aggr_cb looks to have the same bug as the index is being passed as the cpu:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core#n643

> 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)

Agreed on the naming confusion. I'm a fan of using single element
structs to get type safety in code like this. I wonder here if a
for_each_cpu on perf_cpu_map would clean this up best. I'll play with
a v2 patch set that addresses this problem more widely.

Thanks,
Ian

> >                               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),
> >