linux-kernel - Re: [PATCH v2 08/10] perf parse-events: Sort and group parsed events

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:   Fri, 3 Mar 2023 18:22:16 -0800
From:   Namhyung Kim <namhyung@...nel.org>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        "Steinar H. Gunderson" <sesse@...gle.com>,
        Kim Phillips <kim.phillips@....com>,
        Florian Fischer <florian.fischer@...q.space>,
        James Clark <james.clark@....com>,
        Suzuki Poulouse <suzuki.poulose@....com>,
        Sean Christopherson <seanjc@...gle.com>,
        Leo Yan <leo.yan@...aro.org>,
        John Garry <john.g.garry@...cle.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v2 08/10] perf parse-events: Sort and group parsed events

On Thu, Mar 2, 2023 at 5:39 PM Ian Rogers <irogers@...gle.com> wrote:
>
> On Thu, Mar 2, 2023 at 4:37 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > On Thu, Mar 2, 2023 at 1:26 PM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > This change is intended to be a no-op for most current cases, the
> > > default sort order is the order the events were parsed. Where it
> > > varies is in how groups are handled. Previously an uncore and core
> > > event that are grouped would most often cause the group to be removed:
> > >
> > > ```
> > > $ perf stat -e '{instructions,uncore_imc_free_running_0/data_total/}' -a sleep 1
> > > WARNING: grouped events cpus do not match, disabling group:
> > >   anon group { instructions, uncore_imc_free_running_0/data_total/ }
> > > ...
> > > ```
> > >
> > > However, when wildcards are used the events should be re-sorted and
> > > re-grouped in parse_events__set_leader, but this currently fails for
> > > simple examples:
> > >
> > > ```
> > > $ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1
> > >
> > >  Performance counter stats for 'system wide':
> > >
> > >      <not counted> MiB  uncore_imc_free_running/data_read/
> > >      <not counted> MiB  uncore_imc_free_running/data_write/
> > >
> > >        1.000996992 seconds time elapsed
> > > ```
> > >
> > > A futher failure mode, fixed in this patch, is to force topdown events
> > > into a group.
> > >
> > > This change moves sorting the evsels in the evlist after parsing. It
> > > requires parsing to set up groups. First the evsels are sorted
> > > respecting the existing groupings and parse order, but also reordering
> > > to ensure evsels of the same PMU and group appear together. So that
> > > software and aux events respect groups, their pmu_name is taken from
> > > the group leader. The sorting is done with list_sort removing a memory
> > > allocation.
> > >
> > > After sorting a pass is done to correct the group leaders and for
> > > topdown events ensuring they have a group leader.
> > >
> > > This fixes the problems seen before:
> > >
> > > ```
> > > $ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1
> > >
> > >  Performance counter stats for 'system wide':
> > >
> > >             727.42 MiB  uncore_imc_free_running/data_read/
> > >              81.84 MiB  uncore_imc_free_running/data_write/
> > >
> > >        1.000948615 seconds time elapsed
> > > ```
> > >
> > > As well as making groups not fail for cases like:
> > >
> > > ```
> > > $ perf stat -e '{imc_free_running_0/data_total/,imc_free_running_1/data_total/}' -a sleep 1
> > >
> > >  Performance counter stats for 'system wide':
> > >
> > >             256.47 MiB  imc_free_running_0/data_total/
> > >             256.48 MiB  imc_free_running_1/data_total/
> >
> > I didn't expect we can group events from different PMUs.
> > Not sure if it can handle multiplexing properly..
>
> You are right, this example is now working as the sorting and
> regrouping breaks the events into two groups. The rules around
> grouping are complex and Arnaldo mentioned that maybe cases like this
> should be warned about. The problem then is that wildcard and metric
> expansion may naturally produce these cases and we don't want the
> warning. It is something of a shame that the grouping information in
> the perf stat output isn't clearer.

Oh, that means the events are not in a group in this case.
Yeah.. it can be somewhat confusing.  It seems the wildcard
is a kind of exception.  Then we can warn if there's no wildcard?

Thanks,
Namhyung