[PATCH] perf jevents: return potentially empty metrics table

[PATCH] perf jevents: return potentially empty metrics table

[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>
---
 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++) {
-- 
2.39.2

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

[Lucas Stach]

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>
> ---
>  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++) {

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

[Namhyung Kim]

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?

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++) {
> 

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

[Ian Rogers]

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.

Thanks,
Ian

> 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++) {
> >

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

[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.
> 
> Thanks,
> Ian
> 

Yes it looks like the fix should possibly be higher up in the arm64 
pmu_metrics_table__find().

Which set of JSON files are you seeing the issue with exactly Lucas? I 
see that there are multiple versions of i.MX8M metrics.

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++) {
>>>
> 

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

[Lucas Stach]

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++) {
> > > > 
> > 

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

[Ian Rogers]

+John Garry

On Mon, Jul 15, 2024 at 8:54 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> 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.

I don't see that in the commit:
```
 const struct pmu_metrics_table *perf_pmu__find_metrics_table(struct
perf_pmu *pmu)
 {
-        const struct pmu_metrics_table *table = NULL;
-        char *cpuid = perf_pmu__getcpuid(pmu);
-        int i;
+        const struct pmu_events_map *map = map_for_pmu(pmu);

-        /* on some platforms which uses cpus map, cpuid can be NULL for
-         * PMUs other than CORE PMUs.
-         */
-        if (!cpuid)
+        if (!map)
                 return NULL;

-        i = 0;
-        for (;;) {
-                const struct pmu_events_map *map = &pmu_events_map[i++];
-                if (!map->arch)
-                        break;
```
Here the loop is terminated, "table == NULL" and so the empty case
will return NULL.
```
+        if (!pmu)
+                return &map->metric_table;

-                if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
-                        table = &map->metric_table;
-                        break;
-                }
+        for (size_t i = 0; i < map->metric_table.num_pmus; i++) {
+                const struct pmu_table_entry *table_pmu =
&map->metric_table.pmus[i];
+                const char *pmu_name =
&big_c_string[table_pmu->pmu_name.offset];
+
+                if (pmu__name_match(pmu, pmu_name))
+                           return &map->metric_table;
         }
-        free(cpuid);
-        return table;
+        return NULL;
 }
```
In any case this fix won't work on BIG.little where NULL is always
returned, and so a fix higher up the stack is necessary.

Thanks,
Ian

> > > 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++) {
> > > > >
> > >
>