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

[Namhyung Kim]

On Thu, Mar 2, 2023 at 5:39 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Thu, Mar 2, 2023 at 4:37 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > On Thu, Mar 2, 2023 at 1:26 PM Ian Rogers <irogers@xxxxxxxxxx> 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