Re: [PATCH v5 4/4] perf parse-events: Reapply "Prefer sysfs/JSON hardware events over legacy"

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Fri, Jan 10, 2025 at 11:52:47AM -0800, Atish Kumar Patra wrote:
> On Fri, Jan 10, 2025 at 11:40 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, Jan 09, 2025 at 02:21:09PM -0800, Ian Rogers wrote:
> > > Originally posted and merged from:
> > > https://lore.kernel.org/r/20240416061533.921723-10-irogers@google.com
> > > This reverts commit 4f1b067359ac8364cdb7f9fda41085fa85789d0f although
> > > the patch is now smaller due to related fixes being applied in commit
> > > 22a4db3c3603 ("perf evsel: Add alternate_hw_config and use in
> > > evsel__match").
> > > The original commit message was:
> > >
> > > It was requested that RISC-V be able to add events to the perf tool so
> > > the PMU driver didn't need to map legacy events to config encodings:
> > > https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.com/
> > >
> > > This change makes the priority of events specified without a PMU the
> > > same as those specified with a PMU, namely sysfs and JSON events are
> > > checked first before using the legacy encoding.
> >
> > I'm still not convinced why we need this change despite of these
> > troubles.  If it's because RISC-V cannot define the lagacy hardware
> > events in the kernel driver, why not using a different name in JSON and
> 
> When the discussion happened a year back. we tried to avoid defining
> the legacy hardware events in
> the kernel driver. However, we agreed that we have to define it
> anyways for other reasons (legacy usage + virtualization)
> as described here[1]. I have improved the driver in such a way that it
> can handle both legacy events from the
> driver or json file (via this patch) if available. If this patch is
> available, a platform vendor can choose to encode the legacy events in
> json.
> Otherwise, it has to specify them in the driver. I will try to send
> the series today/tomorrow.

Ok, thanks for the update.

> 
> This patch will help avoid proliferation of usage of legacy events in
> the long run. But it is no longer absolutely necessary for RISC-V.
> If this patch is accepted, there is a hope that we can get rid of the
> specifying encodings in the driver in the distant future. However, we
> have
> to define them in the driver for reasons described in[1].
> 
> [1] https://lore.kernel.org/lkml/20241026121758.143259-1-irogers@google.com/T/#m653a6b98919a365a361a698032502bd26af9f6ba
> > ask users to use the name specifically?  Something like:
> >
> >   $ perf record -e riscv-cycles ...
> >
> 
> That was the first alternative I proposed back in 2022 plumbers :).

I see, sorry for missing the earlier discussion.

Thanks,
Namhyung


> But it was concluded that we don't want users to learn new ways
> of running perf in RISC-V which makes sense to me as well.