Re: [PATCH v5 3/4] perf record: Skip don't fail for events that don't open

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Jan 09, 2025 at 08:44:38PM -0800, Ian Rogers wrote:
> On Thu, Jan 9, 2025 at 5:25 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, Jan 09, 2025 at 02:21:08PM -0800, Ian Rogers wrote:
> > > Whilst for many tools it is an expected behavior that failure to open
> > > a perf event is a failure, ARM decided to name PMU events the same as
> > > legacy events and then failed to rename such events on a server uncore
> > > SLC PMU. As perf's default behavior when no PMU is specified is to
> > > open the event on all PMUs that advertise/"have" the event, this
> > > yielded failures when trying to make the priority of legacy and
> > > sysfs/json events uniform - something requested by RISC-V and ARM. A
> > > legacy event user on ARM hardware may find their event opened on an
> > > uncore PMU which for perf record will fail. Arnaldo suggested skipping
> > > such events which this patch implements. Rather than have the skipping
> > > conditional on running on ARM, the skipping is done on all
> > > architectures as such a fundamental behavioral difference could lead
> > > to problems with tools built/depending on perf.
> > >
> > > An example of perf record failing to open events on x86 is:
> > > ```
> > > $ perf record -e data_read,cycles,LLC-prefetch-read -a sleep 0.1
> > > Error:
> > > Failure to open event 'data_read' on PMU 'uncore_imc_free_running_0' which will be removed.
> > > The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (data_read).
> > > "dmesg | grep -i perf" may provide additional information.
> > >
> > > Error:
> > > Failure to open event 'data_read' on PMU 'uncore_imc_free_running_1' which will be removed.
> > > The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (data_read).
> > > "dmesg | grep -i perf" may provide additional information.
> > >
> > > Error:
> > > Failure to open event 'LLC-prefetch-read' on PMU 'cpu' which will be removed.
> > > The LLC-prefetch-read event is not supported.
> > > [ perf record: Woken up 1 times to write data ]
> > > [ perf record: Captured and wrote 2.188 MB perf.data (87 samples) ]
> >
> > I'm afraid this can be too noisy.
> 
> The intention is to be noisy:
> 1) it matches the existing behavior, anything else is potentially a regression;

Well.. I think you're changing the behavior. :)  Also currently it just
fails on the first event so it won't be too much noisy.

  $ perf record -e data_read,data_write,LLC-prefetch-read -a sleep 0.1
  event syntax error: 'data_read,data_write,LLC-prefetch-read'
                       \___ Bad event name
  
  Unable to find event on a PMU of 'data_read'
  Run 'perf list' for a list of valid events
  
   Usage: perf record [<options>] [<command>]
      or: perf record [<options>] -- <command> [<options>]
  
      -e, --event <event>   event selector. use 'perf list' to list available events


> 2) it only happens if trying to record on a PMU/event that doesn't
> support recording, something that is currently an error and so we're
> not motivated to change the behavior as no-one should be using it;

It was caught by Linus, so we know at least one (very important) user.


> 3) for the wildcard case the only offender is ARM's SLC PMU and the
> appropriate fix there has always been to make the CPU cycle's event
> name match the bus_cycles event name by calling it cpu_cycles -
> something that doesn't conflict with a core PMU event name, the thing
> that has introduced all these problems, patches, long email exchanges,
> unfixed inconsistencies, etc.. If the errors aren't noisy then there
> is little motivation for the ARM SLC PMU's event name to be fixed.

I understand your concern but I'm not sure it's the best way to fix the
issue.

James, Leo, is there any chance you can rename the SLC cycles event in
the kernel driver?

Thanks,
Namhyung