* [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match
@ 2026-03-26 22:44 Ian Rogers
2026-03-30 14:44 ` James Clark
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2026-03-26 22:44 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Alexander Shishkin, Jiri Olsa, Ian Rogers,
Adrian Hunter, James Clark, Leo Yan, Thomas Falcon,
linux-perf-users, linux-kernel
Only uncore PMUs can have an identifier, so add an optimized
perf_pmus__scan routine for that case to avoid all PMU types being
created.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/metricgroup.c | 8 ++------
tools/perf/util/pmus.c | 18 +++++++++++++++++-
tools/perf/util/pmus.h | 1 +
3 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 7e39d469111b..769b38400832 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -410,13 +410,9 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
if (!pm->metric_expr || !pm->compat)
return 0;
- while ((pmu = perf_pmus__scan(pmu))) {
-
- if (!pmu->id || !pmu_uncore_identifier_match(pm->compat, pmu->id))
- continue;
-
+ while ((pmu = perf_pmus__scan_for_uncore_id(pmu, pm->compat)))
return d->fn(pm, table, d->data);
- }
+
return 0;
}
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 98be2eb8f1f0..9859d7056fb7 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -409,7 +409,7 @@ struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const c
if (!pmu) {
/*
* Core PMUs, other sysfs PMUs and tool PMU can have any name or
- * aren't wother optimizing for.
+ * aren't worth optimizing for.
*/
unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_CORE_MASK |
PERF_TOOL_PMU_TYPE_PE_OTHER_MASK |
@@ -486,6 +486,22 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
return NULL;
}
+struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat)
+{
+ if (!pmu) {
+ /* Only uncore PMUs can have identifiers. */
+ unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_OTHER_MASK;
+
+ pmu_read_sysfs(to_read_pmus);
+ pmu = list_prepare_entry(pmu, &other_pmus, list);
+ }
+ list_for_each_entry_continue(pmu, &other_pmus, list) {
+ if (pmu->id && pmu_uncore_identifier_match(compat, pmu->id))
+ return pmu;
+ }
+ return NULL;
+}
+
const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
{
struct perf_pmu *pmu = NULL;
diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
index 7cb36863711a..0d55edb3f2fc 100644
--- a/tools/perf/util/pmus.h
+++ b/tools/perf/util/pmus.h
@@ -23,6 +23,7 @@ struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu);
struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu);
struct perf_pmu *perf_pmus__scan_for_event(struct perf_pmu *pmu, const char *event);
struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const char *wildcard);
+struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat);
const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str);
--
2.53.0.1018.g2bb0e51243-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match
2026-03-26 22:44 [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match Ian Rogers
@ 2026-03-30 14:44 ` James Clark
2026-03-31 6:30 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: James Clark @ 2026-03-30 14:44 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Leo Yan, Thomas Falcon, linux-perf-users, linux-kernel
On 26/03/2026 10:44 pm, Ian Rogers wrote:
> Only uncore PMUs can have an identifier, so add an optimized
> perf_pmus__scan routine for that case to avoid all PMU types being
> created.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: James Clark <james.clark@linaro.org>
> ---
> tools/perf/util/metricgroup.c | 8 ++------
> tools/perf/util/pmus.c | 18 +++++++++++++++++-
> tools/perf/util/pmus.h | 1 +
> 3 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 7e39d469111b..769b38400832 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -410,13 +410,9 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
> if (!pm->metric_expr || !pm->compat)
> return 0;
>
> - while ((pmu = perf_pmus__scan(pmu))) {
> -
> - if (!pmu->id || !pmu_uncore_identifier_match(pm->compat, pmu->id))
> - continue;
> -
> + while ((pmu = perf_pmus__scan_for_uncore_id(pmu, pm->compat)))
> return d->fn(pm, table, d->data);
> - }
> +
> return 0;
> }
>
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 98be2eb8f1f0..9859d7056fb7 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -409,7 +409,7 @@ struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const c
> if (!pmu) {
> /*
> * Core PMUs, other sysfs PMUs and tool PMU can have any name or
> - * aren't wother optimizing for.
> + * aren't worth optimizing for.
> */
> unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_CORE_MASK |
> PERF_TOOL_PMU_TYPE_PE_OTHER_MASK |
> @@ -486,6 +486,22 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> return NULL;
> }
>
> +struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat)
> +{
> + if (!pmu) {
> + /* Only uncore PMUs can have identifiers. */
> + unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_OTHER_MASK;
> +
> + pmu_read_sysfs(to_read_pmus);
> + pmu = list_prepare_entry(pmu, &other_pmus, list);
> + }
> + list_for_each_entry_continue(pmu, &other_pmus, list) {
> + if (pmu->id && pmu_uncore_identifier_match(compat, pmu->id))
> + return pmu;
> + }
> + return NULL;
> +}
> +
> const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> {
> struct perf_pmu *pmu = NULL;
> diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
> index 7cb36863711a..0d55edb3f2fc 100644
> --- a/tools/perf/util/pmus.h
> +++ b/tools/perf/util/pmus.h
> @@ -23,6 +23,7 @@ struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu);
> struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu);
> struct perf_pmu *perf_pmus__scan_for_event(struct perf_pmu *pmu, const char *event);
> struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const char *wildcard);
> +struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat);
>
> const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match
2026-03-30 14:44 ` James Clark
@ 2026-03-31 6:30 ` Namhyung Kim
2026-03-31 14:05 ` Ian Rogers
0 siblings, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2026-03-31 6:30 UTC (permalink / raw)
To: James Clark
Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Adrian Hunter, Leo Yan,
Thomas Falcon, linux-perf-users, linux-kernel
On Mon, Mar 30, 2026 at 03:44:21PM +0100, James Clark wrote:
>
>
> On 26/03/2026 10:44 pm, Ian Rogers wrote:
> > Only uncore PMUs can have an identifier, so add an optimized
> > perf_pmus__scan routine for that case to avoid all PMU types being
> > created.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> Reviewed-by: James Clark <james.clark@linaro.org>
>
> > ---
> > tools/perf/util/metricgroup.c | 8 ++------
> > tools/perf/util/pmus.c | 18 +++++++++++++++++-
> > tools/perf/util/pmus.h | 1 +
> > 3 files changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 7e39d469111b..769b38400832 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -410,13 +410,9 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
> > if (!pm->metric_expr || !pm->compat)
> > return 0;
> > - while ((pmu = perf_pmus__scan(pmu))) {
> > -
> > - if (!pmu->id || !pmu_uncore_identifier_match(pm->compat, pmu->id))
> > - continue;
> > -
> > + while ((pmu = perf_pmus__scan_for_uncore_id(pmu, pm->compat)))
> > return d->fn(pm, table, d->data);
Sashiko review: it's natural to convert it to 'if'.
Thanks,
Namhyung
> > - }
> > +
> > return 0;
> > }
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index 98be2eb8f1f0..9859d7056fb7 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -409,7 +409,7 @@ struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const c
> > if (!pmu) {
> > /*
> > * Core PMUs, other sysfs PMUs and tool PMU can have any name or
> > - * aren't wother optimizing for.
> > + * aren't worth optimizing for.
> > */
> > unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_CORE_MASK |
> > PERF_TOOL_PMU_TYPE_PE_OTHER_MASK |
> > @@ -486,6 +486,22 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > return NULL;
> > }
> > +struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat)
> > +{
> > + if (!pmu) {
> > + /* Only uncore PMUs can have identifiers. */
> > + unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_OTHER_MASK;
> > +
> > + pmu_read_sysfs(to_read_pmus);
> > + pmu = list_prepare_entry(pmu, &other_pmus, list);
> > + }
> > + list_for_each_entry_continue(pmu, &other_pmus, list) {
> > + if (pmu->id && pmu_uncore_identifier_match(compat, pmu->id))
> > + return pmu;
> > + }
> > + return NULL;
> > +}
> > +
> > const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> > {
> > struct perf_pmu *pmu = NULL;
> > diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
> > index 7cb36863711a..0d55edb3f2fc 100644
> > --- a/tools/perf/util/pmus.h
> > +++ b/tools/perf/util/pmus.h
> > @@ -23,6 +23,7 @@ struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu);
> > struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu);
> > struct perf_pmu *perf_pmus__scan_for_event(struct perf_pmu *pmu, const char *event);
> > struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const char *wildcard);
> > +struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat);
> > const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match
2026-03-31 6:30 ` Namhyung Kim
@ 2026-03-31 14:05 ` Ian Rogers
2026-04-01 3:51 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2026-03-31 14:05 UTC (permalink / raw)
To: Namhyung Kim
Cc: James Clark, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, Leo Yan, Thomas Falcon, linux-perf-users,
linux-kernel
On Mon, Mar 30, 2026 at 11:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Mar 30, 2026 at 03:44:21PM +0100, James Clark wrote:
> >
> >
> > On 26/03/2026 10:44 pm, Ian Rogers wrote:
> > > Only uncore PMUs can have an identifier, so add an optimized
> > > perf_pmus__scan routine for that case to avoid all PMU types being
> > > created.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> >
> > Reviewed-by: James Clark <james.clark@linaro.org>
> >
> > > ---
> > > tools/perf/util/metricgroup.c | 8 ++------
> > > tools/perf/util/pmus.c | 18 +++++++++++++++++-
> > > tools/perf/util/pmus.h | 1 +
> > > 3 files changed, 20 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > index 7e39d469111b..769b38400832 100644
> > > --- a/tools/perf/util/metricgroup.c
> > > +++ b/tools/perf/util/metricgroup.c
> > > @@ -410,13 +410,9 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
> > > if (!pm->metric_expr || !pm->compat)
> > > return 0;
> > > - while ((pmu = perf_pmus__scan(pmu))) {
> > > -
> > > - if (!pmu->id || !pmu_uncore_identifier_match(pm->compat, pmu->id))
> > > - continue;
> > > -
> > > + while ((pmu = perf_pmus__scan_for_uncore_id(pmu, pm->compat)))
> > > return d->fn(pm, table, d->data);
>
> Sashiko review: it's natural to convert it to 'if'.
Yeah, I saw and disagreed. The pattern with the "while ((pmu =
perf_pmus__scan...(pmu)))" functions is for them to be a while loop.
With an "if" it would read:
```
pmu = perf_pmus__scan...(pmu);
if (pmu)
return ...
```
and we lose the consistency of having while loops in the code.
Thanks,
Ian
> Thanks,
> Namhyung
>
>
> > > - }
> > > +
> > > return 0;
> > > }
> > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > > index 98be2eb8f1f0..9859d7056fb7 100644
> > > --- a/tools/perf/util/pmus.c
> > > +++ b/tools/perf/util/pmus.c
> > > @@ -409,7 +409,7 @@ struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const c
> > > if (!pmu) {
> > > /*
> > > * Core PMUs, other sysfs PMUs and tool PMU can have any name or
> > > - * aren't wother optimizing for.
> > > + * aren't worth optimizing for.
> > > */
> > > unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_CORE_MASK |
> > > PERF_TOOL_PMU_TYPE_PE_OTHER_MASK |
> > > @@ -486,6 +486,22 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > > return NULL;
> > > }
> > > +struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat)
> > > +{
> > > + if (!pmu) {
> > > + /* Only uncore PMUs can have identifiers. */
> > > + unsigned int to_read_pmus = PERF_TOOL_PMU_TYPE_PE_OTHER_MASK;
> > > +
> > > + pmu_read_sysfs(to_read_pmus);
> > > + pmu = list_prepare_entry(pmu, &other_pmus, list);
> > > + }
> > > + list_for_each_entry_continue(pmu, &other_pmus, list) {
> > > + if (pmu->id && pmu_uncore_identifier_match(compat, pmu->id))
> > > + return pmu;
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> > > {
> > > struct perf_pmu *pmu = NULL;
> > > diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
> > > index 7cb36863711a..0d55edb3f2fc 100644
> > > --- a/tools/perf/util/pmus.h
> > > +++ b/tools/perf/util/pmus.h
> > > @@ -23,6 +23,7 @@ struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu);
> > > struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu);
> > > struct perf_pmu *perf_pmus__scan_for_event(struct perf_pmu *pmu, const char *event);
> > > struct perf_pmu *perf_pmus__scan_matching_wildcard(struct perf_pmu *pmu, const char *wildcard);
> > > +struct perf_pmu *perf_pmus__scan_for_uncore_id(struct perf_pmu *pmu, const char *compat);
> > > const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str);
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match
2026-03-31 14:05 ` Ian Rogers
@ 2026-04-01 3:51 ` Namhyung Kim
0 siblings, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2026-04-01 3:51 UTC (permalink / raw)
To: Ian Rogers
Cc: James Clark, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, Leo Yan, Thomas Falcon, linux-perf-users,
linux-kernel
On Tue, Mar 31, 2026 at 07:05:58AM -0700, Ian Rogers wrote:
> On Mon, Mar 30, 2026 at 11:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Mar 30, 2026 at 03:44:21PM +0100, James Clark wrote:
> > >
> > >
> > > On 26/03/2026 10:44 pm, Ian Rogers wrote:
> > > > Only uncore PMUs can have an identifier, so add an optimized
> > > > perf_pmus__scan routine for that case to avoid all PMU types being
> > > > created.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > >
> > > Reviewed-by: James Clark <james.clark@linaro.org>
> > >
> > > > ---
> > > > tools/perf/util/metricgroup.c | 8 ++------
> > > > tools/perf/util/pmus.c | 18 +++++++++++++++++-
> > > > tools/perf/util/pmus.h | 1 +
> > > > 3 files changed, 20 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > > index 7e39d469111b..769b38400832 100644
> > > > --- a/tools/perf/util/metricgroup.c
> > > > +++ b/tools/perf/util/metricgroup.c
> > > > @@ -410,13 +410,9 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
> > > > if (!pm->metric_expr || !pm->compat)
> > > > return 0;
> > > > - while ((pmu = perf_pmus__scan(pmu))) {
> > > > -
> > > > - if (!pmu->id || !pmu_uncore_identifier_match(pm->compat, pmu->id))
> > > > - continue;
> > > > -
> > > > + while ((pmu = perf_pmus__scan_for_uncore_id(pmu, pm->compat)))
> > > > return d->fn(pm, table, d->data);
> >
> > Sashiko review: it's natural to convert it to 'if'.
>
> Yeah, I saw and disagreed. The pattern with the "while ((pmu =
> perf_pmus__scan...(pmu)))" functions is for them to be a while loop.
> With an "if" it would read:
> ```
> pmu = perf_pmus__scan...(pmu);
> if (pmu)
> return ...
> ```
> and we lose the consistency of having while loops in the code.
I understand it's a pattern to scan PMUs but having an unused
variable assignment in a while loop with a return statement seems
unnatural and maybe someone would send a patch for it later.
At least we can add a comment saying it's intended?
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-04-01 3:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-26 22:44 [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match Ian Rogers
2026-03-30 14:44 ` James Clark
2026-03-31 6:30 ` Namhyung Kim
2026-03-31 14:05 ` Ian Rogers
2026-04-01 3:51 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox