Re: [PATCH] perf jevents: return potentially empty metrics table

[Lucas Stach <l.stach@pengutronix.de>]

Am Montag, dem 15.07.2024 um 16:12 +0100 schrieb James Clark:
> 
> On 12/07/2024 10:54 pm, Ian Rogers wrote:
> > On Wed, Jul 3, 2024 at 3:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > 
> > > Hello,
> > > 
> > > On Mon, Jul 01, 2024 at 07:19:55PM +0200, Lucas Stach wrote:
> > > > Hi,
> > > > 
> > > > has this patch fallen through the cracks? It fixes a functional
> > > > regression where the DDR controller metrics are completely unavailable
> > > > on all i.MX8M* systems and thus would be nice if someone could have a
> > > > look.
> > > > 
> > > > Regards,
> > > > Lucas
> > > > 
> > > > Am Freitag, dem 31.05.2024 um 21:44 +0200 schrieb Lucas Stach:
> > > > > Don't return NULL when a empty (num_pmus = 0) metrics table is encountered,
> > > > > as this causes many of the users to bail out, which will skip matching any
> > > > > potentially existing sys metrics later on. Instead return the empty table
> > > > > which will be handled properly by the iterators and allows matching to
> > > > > continue.
> > > > > 
> > > > > This fixes metrics reporting on systems where only the sys, but not the
> > > > > core PMUs have metrics defined.
> > > > > 
> > > > > Fixes: f20c15d13f01 ("perf pmu-events: Remember the perf_events_map for a PMU")
> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > 
> > > Looks ok to me.  Ian?
> > 
> > Sorry I missed this, I expect to see patches on LKML and so my email
> > filters placed this elsewhere.
> > 
> > The change doesn't make sense. A search is requested with a PMU and
> > you return success (the table) when there is no PMU match. If this
> > behavior were expected then NULL should be passed as a PMU - this is
> > the behavior in util/pmu.c and ARM is the only architecture to pass a
> > PMU. The ARM pmu_metrics_table__find also specifically breaks
> > heterogeneous (BIG.little) systems and this change won't impact/fix
> > that. Perhaps there is some context I lack.
> > 

The missing context is probably the call chain that I somehow omitted
and needed to piece together again right now. The issue I'm try to fix
here is perf stat not being able to measure metrics from a system-wide
PMU (the i.MX8M* ddrc) only.

The underlying issue is that when there is no non-systemwide PMU,
perf_pmu__find_metrics_table will return NULL after f20c15d13f01. This
causes metricgroup__parse_groups to drop out early. However, we need
this function to call the chain parse_groups() ->
metricgroup__add_metric_list -> metricgroup__add_metric, as only at
this point system wide metrics, like the ddrc, will be added to the
list.

It is correct to drop out early, as those functions can not be called
with a NULL parameter. Before f20c15d13f01 and after the patch
discussed here perf_pmu__find_metrics_table() will simply return a
table with 0 entries. The called functions are fine with this, as they
iterate the entries and the execution flow is able to reach the point
where the system wide events are added.

> > Thanks,
> > Ian
> > 
> 
> Yes it looks like the fix should possibly be higher up in the arm64 
> pmu_metrics_table__find().
> 
Maybe someone can figure out how to fix this in a better way higher up
the call chain with the description above. However, my fix looked quite
straight forward to me, as it simply restores the behavior before
f20c15d13f01.

> Which set of JSON files are you seeing the issue with exactly Lucas? I 
> see that there are multiple versions of i.MX8M metrics.
> 
It was first observed on a i.MX8MP, but the issue is also reproducible
on i.MX8MM. I guess it doesn't matter which SoC is used specifically,
the key point is that the sampling isn't able to match any specific
PMU, but there are still system wide metrics.

Regards,
Lucas

> James
> 
> > > Thanks,
> > > Namhyung
> > > 
> > > 
> > > > > ---
> > > > >   tools/perf/pmu-events/jevents.py | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> > > > > index e42efc16723e..0a1ed9ee1429 100755
> > > > > --- a/tools/perf/pmu-events/jevents.py
> > > > > +++ b/tools/perf/pmu-events/jevents.py
> > > > > @@ -1081,7 +1081,7 @@ const struct pmu_metrics_table *perf_pmu__find_metrics_table(struct perf_pmu *pm
> > > > >           if (!map)
> > > > >                   return NULL;
> > > > > 
> > > > > -        if (!pmu)
> > > > > +        if (!pmu || !map->metric_table.num_pmus)
> > > > >                   return &map->metric_table;
> > > > > 
> > > > >           for (size_t i = 0; i < map->metric_table.num_pmus; i++) {
> > > > 
> >