Re: [PATCH v1 1/2] perf pmu: Make pmu_alias_terms weak again

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Oct 30, 2025 at 08:08:02AM -0700, Ian Rogers wrote:
> On Wed, Oct 29, 2025 at 10:47 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Wed, Oct 29, 2025 at 03:26:37PM -0700, Ian Rogers wrote:
> > > The terms for a json event should be weak so they don't override
> > > command line options.
> >
> > Can you please give an example command line and the error?
> 
> Sure, it is also covered in the test.

Yep, but lazy maintainers want to see it in the commit message. :)

> 
> Before:
> ```
> $ perf record -vv -c 1000 -e uops_issued.any -o /dev/null true 2>&1
> |grep "{ sample_period, sample_freq }"
>  { sample_period, sample_freq }   200003
>  { sample_period, sample_freq }   2000003
>  { sample_period, sample_freq }   1000
> ```
> 
> After:
> ```
> $ perf record -vv -c 1000 -e uops_issued.any -o /dev/null true 2>&1
> |grep "{ sample_period, sample_freq }"
>  { sample_period, sample_freq }   1000
>  { sample_period, sample_freq }   1000
>  { sample_period, sample_freq }   1000
> ```

Thanks, it ignored command line options.  I can see the default period
is in the JSON description.

  $ perf list -j | grep -C 5 uops_issued.any
  	"Encoding": "cpu/event=0xb1,period=2000003,umask=0x10/"
  },
  {
  	"Unit": "cpu",
  	"Topic": "pipeline",
  	"EventName": "uops_issued.any",
  	"EventType": "Kernel PMU event",
  	"BriefDescription": "Uops that RAT issues to RS",
  	"PublicDescription": "Counts the number of uops that the Resource Allocation Table (RAT) issues to the Reservation Station (RS)",
  	"Encoding": "cpu/event=0xe,period=2000003,umask=1/"
  },

It'd be great if you could include this kind of info in the commit log
so that others can understand the problem clearly and test the patch
easily.

Thanks,
Namhyung

> > >
> > > Fixes: 84bae3af20d0 ("perf pmu: Don't eagerly parse event terms")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/pmu.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index d597263fab4f..f14f2a12d061 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -802,6 +802,7 @@ static int pmu_aliases_parse_eager(struct perf_pmu *pmu, int sysfs_fd)
> > >  static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms)
> > >  {
> > >       struct parse_events_terms alias_terms;
> > > +     struct parse_events_term *term;
> > >       int ret;
> > >
> > >       parse_events_terms__init(&alias_terms);
> > > @@ -812,6 +813,13 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms
> > >               parse_events_terms__exit(&alias_terms);
> > >               return ret;
> > >       }
> > > +     list_for_each_entry(term, &alias_terms.terms, list) {
> > > +             /*
> > > +              * Weak terms don't override command line options,
> > > +              * which we don't want for implicit terms in aliases.
> > > +              */
> > > +             term->weak = true;
> > > +     }
> > >       list_splice_init(&alias_terms.terms, terms);
> > >       parse_events_terms__exit(&alias_terms);
> > >       return 0;
> > > --
> > > 2.51.1.851.g4ebd6896fd-goog
> > >