Re: [PATCH v2] perf stat: Introduce skippable evsels

[Ian Rogers]

On Wed, Apr 19, 2023 at 5:31 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 2023-04-18 9:00 p.m., Ian Rogers wrote:
> > On Tue, Apr 18, 2023 at 5:12 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >>
> >> On Tue, Apr 18, 2023 at 2:51 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
> >>>
> >>>
> >>>
> >>> On 2023-04-18 4:08 p.m., Ian Rogers wrote:
> >>>> On Tue, Apr 18, 2023 at 11:19 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 2023-04-18 11:43 a.m., Ian Rogers wrote:
> >>>>>> On Tue, Apr 18, 2023 at 6:03 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 2023-04-17 2:13 p.m., Ian Rogers wrote:
> >>>>>>>> The json TopdownL1 is enabled if present unconditionally for perf stat
> >>>>>>>> default. Enabling it on Skylake has multiplexing as TopdownL1 on
> >>>>>>>> Skylake has multiplexing unrelated to this change - at least on the
> >>>>>>>> machine I was testing on. We can remove the metric group TopdownL1 on
> >>>>>>>> Skylake so that we don't enable it by default, there is still the
> >>>>>>>> group TmaL1. To me, disabling TopdownL1 seems less desirable than
> >>>>>>>> running with multiplexing - previously to get into topdown analysis
> >>>>>>>> there has to be knowledge that "perf stat -M TopdownL1" is the way to
> >>>>>>>> do this.
> >>>>>>>
> >>>>>>> To be honest, I don't think it's a good idea to remove the TopdownL1. We
> >>>>>>> cannot remove it just because the new way cannot handle it. The perf
> >>>>>>> stat default works well until 6.3-rc7. It's a regression issue of the
> >>>>>>> current perf-tools-next.
> >>>>>>
> >>>>>> I'm not so clear it is a regression to consistently add TopdownL1 for
> >>>>>> all architectures supporting it.
> >>>>>
> >>>>>
> >>>>> Breaking the perf stat default is a regression.
> >>>>
> >>>> Breaking is overstating the use of multiplexing. The impact is less
> >>>> accuracy in the IPC and branch misses default metrics,
> >>>
> >>> Inaccuracy is a breakage for the default.
> >>
> >> Can you present a case where this matters? The events are already not
> >> grouped and so inaccurate for metrics.
> >
> > Removing CPUs without perf metrics from the TopdownL1 metric group is
> > implemented here:
> > https://lore.kernel.org/lkml/20230419005423.343862-6-irogers@xxxxxxxxxx/
> > Note, this applies to pre-Icelake and atom CPUs as these also lack
> > perf metric (aka topdown) events.
> >
>
> That may give the end user the impression that the pre-Icelake doesn't
> support the Topdown Level1 events, which is not true.
>
> I think perf should either keep it for all Intel platforms which
> supports tma_L1_group, or remove the TopdownL1 name entirely for Intel
> platform (let the end user use the tma_L1_group and the name exposed by
> the kernel as before.).

How does this work on hybrid systems? We will enable TopdownL1 because
of the presence of perf metric (aka topdown) events but this will also
enable TopdownL1 on the atom core.

>
> > With that change I don't have a case that requires skippable evsels,
> > and so we can take that series with patch 6 over the v1 of that series
> > with this change.
> >
>
> I'm afraid this is not the only problem the commit 94b1a603fca7 ("perf
> stat: Add TopdownL1 metric as a default if present") in the
> perf-tools-next branch introduced.
>
> The topdown L2 in the perf stat default on SPR and big core of the ADL
> is still missed. I don't see a possible fix for this on the current
> perf-tools-next branch.

I thought in its current state the json metrics for TopdownL2 on SPR
have multiplexing. Given L1 is used to drill down to L2, it seems odd
to start on L2, but given L1 is used to compute the thresholds for L2,
this should be to have both L1 and L2 on these platforms. However,
that doesn't work as you don't want multiplexing.

This all seems backward to avoid potential multiplexing on branch miss
rate and IPC, just always having TopdownL1 seems cleanest with the
skippable evsels working around the permissions issue - as put forward
in this patch. Possibly adding L2 metrics on ADL/SPR, but only once
the multiplexing issue is resolved.

Thanks,
Ian

> Thanks,
> Kan