Re: [PATCH v5 11/24] perf vendor events: Update/add Graniterapids events/metrics

[Ian Rogers]

On Thu, Feb 6, 2025 at 10:59 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
> On 2025-02-06 1:53 p.m., Liang, Kan wrote:
> >>> However, when I dump the debug information,
> >>> ./perf stat -M tma_frontend_bound -vvv
> >>>
> >>> I got below debug information. I have no idea where the slot is from.
> >>> It seems the perf code mess up the p-core metrics with the e-core
> >>> metrics. But why only slot?
> >>> It seems a bug of perf tool.
> >>>
> >>> found event cpu_atom@CPU_CLK_UNHALTED.CORE@
> >>> found event cpu_atom@TOPDOWN_FE_BOUND.ALL@
> >>> found event slots
> >>> Parsing metric events
> >>>
> >>> '{cpu_atom/CPU_CLK_UNHALTED.CORE,metric-id=cpu_atom!3CPU_CLK_UNHALTED.CORE!3/,cpu_atom/TOPDOWN_FE_BOUND.ALL,metric-id=cpu_atom!3TOPDOWN_FE_BOUND.ALL!3/,slots/metric-id=slots/}:W'
> > It because the perf adds "slot" as a tool event for the e-core Topdown
> > metrics.
> > There is no "slot" event for e-core.
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
> > tools/perf/util/metricgroup.c#n1481
> >
> > I will check why "slot" event is added as a tool event for e-core?
> > That doesn't make sense.
>
>
> BTW: I checked the list of the tool events. Except the slots, others are
> all SW (or non-HW) events which should always be available.
> Slots is a HW event and only be available for p-core.
> It doesn't look like a good fit here.
>
> static const char *const tool_pmu__event_names[TOOL_PMU__EVENT_MAX] = {
>         NULL,
>         "duration_time",
>         "user_time",
>         "system_time",
>         "has_pmem",
>         "num_cores",
>         "num_cpus",
>         "num_cpus_online",
>         "num_dies",
>         "num_packages",
>         "slots",
>         "smt_on",
>         "system_tsc_freq",
> };

IIRC it was added on metric changes I wasn't a reviewer on:
https://lore.kernel.org/r/1673940573-90503-2-git-send-email-renyu.zj@xxxxxxxxxxxxxxxxx
and then carried forward by me. The event should only be enabled on aarch64:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/tool_pmu.c?h=perf-tools-next#n38
Where is the slots event added to the metric expr (expression) string?
The metric should have events pulled out, parsed and
parse_events__sort_events_and_fix_groups deal with related issues to
this. I thought it was a requirement that the slots be in the metric -
or else why is the converter script injecting them? :-)

Thanks,
Ian