Re: [PATCH v1] perf metricgroup: Avoid scanning unnecessary PMUs for identifier match

[Namhyung Kim]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > >
> > > Reviewed-by: James Clark <james.clark@xxxxxxxxxx>
> > >
> > > > ---
> > > >   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