At the RISC-V summit the topic of avoiding event data being in the
RISC-V PMU kernel driver came up. There is a preference for sysfs/JSON
events being the priority when no PMU is provided so that legacy
events maybe supported via json. Originally Mark Rutland also
expressed at LPC 2023 that doing this would resolve bugs on ARM Apple
M? processors, but James Clark more recently tested this and believes
the driver issues there may not have existed or have been resolved. In
any case, it is inconsistent that with a PMU event names avoid legacy
encodings, but when wildcarding PMUs (ie without a PMU with the event
name) the legacy encodings have priority.

The patch doing this work was reverted in a v6.10 release candidate
as, even though the patch was posted for weeks and had been on
linux-next for weeks without issue, Linus was in the habit of using
explicit legacy events with unsupported precision options on his
Neoverse-N1. This machine has SLC PMU events for bus and CPU cycles
where ARM decided to call the events bus_cycles and cycles, the latter
being also a legacy event name. ARM haven't renamed the cycles event
to a more consistent cpu_cycles and avoided the problem. With these
changes the problematic event will now be skipped, a large warning
produced, and perf record will continue for the other PMU events. This
solution was proposed by Arnaldo.

Two minor changes have been added to help with the error message and
to work around issues occurring with "perf stat metrics (shadow stat)
test".

The patches have only been tested on my x86 non-hybrid laptop.

v2: Rebase and add tested-by tags from James Clark, Leo Yan and Atish
    Patra who have tested on RISC-V and ARM CPUs, including the
    problem case from before.

Ian Rogers (4):
  perf evsel: Add pmu_name helper
  perf stat: Fix find_stat for mixed legacy/non-legacy events
  perf record: Skip don't fail for events that don't open
  perf parse-events: Reapply "Prefer sysfs/JSON hardware events over
    legacy"

 tools/perf/builtin-record.c    | 22 +++++++---
 tools/perf/util/evsel.c        | 10 +++++
 tools/perf/util/evsel.h        |  1 +
 tools/perf/util/parse-events.c | 26 +++++++++---
 tools/perf/util/parse-events.l | 76 +++++++++++++++++-----------------
 tools/perf/util/parse-events.y | 60 ++++++++++++++++++---------
 tools/perf/util/pmus.c         | 20 +++++++--
 tools/perf/util/stat-shadow.c  |  3 +-
 8 files changed, 145 insertions(+), 73 deletions(-)

-- 
2.47.0.277.g8800431eea-goog

> On 13 Nov 2024, at 6:49 AM, Ian Rogers <irogers@google.com> wrote:
> 
> At the RISC-V summit the topic of avoiding event data being in the
> RISC-V PMU kernel driver came up. There is a preference for sysfs/JSON
> events being the priority when no PMU is provided so that legacy
> events maybe supported via json. Originally Mark Rutland also
> expressed at LPC 2023 that doing this would resolve bugs on ARM Apple
> M? processors, but James Clark more recently tested this and believes
> the driver issues there may not have existed or have been resolved. In
> any case, it is inconsistent that with a PMU event names avoid legacy
> encodings, but when wildcarding PMUs (ie without a PMU with the event
> name) the legacy encodings have priority.
> 
> The patch doing this work was reverted in a v6.10 release candidate
> as, even though the patch was posted for weeks and had been on
> linux-next for weeks without issue, Linus was in the habit of using
> explicit legacy events with unsupported precision options on his
> Neoverse-N1. This machine has SLC PMU events for bus and CPU cycles
> where ARM decided to call the events bus_cycles and cycles, the latter
> being also a legacy event name. ARM haven't renamed the cycles event
> to a more consistent cpu_cycles and avoided the problem. With these
> changes the problematic event will now be skipped, a large warning
> produced, and perf record will continue for the other PMU events. This
> solution was proposed by Arnaldo.
> 
> Two minor changes have been added to help with the error message and
> to work around issues occurring with "perf stat metrics (shadow stat)
> test".
> 
> The patches have only been tested on my x86 non-hybrid laptop.

Hi,
After applying this patch series,we observed a regression while running the perf test suite on powerpc system. Specifically, test case for "Check branch stack sampling" now fails.
Upon investigation, identified that patch "perf record: Skip don't fail for events that don't open"  is causing the breakage. This test case uses branch-filter as "save_type" and it is supposed to be skipped in powerpc. 
Snippet of code:

skip the test if the hardware doesn't support branch stack sampling
 and if the architecture doesn't support filter types: any,save_type,u
if ! perf record -o- --no-buildid --branch-filter any,save_type,u -- true > /dev/null 2>&1 ; then
    echo "skip: system doesn't support filter types: any,save_type,u"
    exit 2
fi

Before applying the patch, running the command:
./perf record -o- --no-buildid --branch-filter any,save_type,u -- true  
cycles:PH: PMU Hardware or event type doesn't support branch stack sampling.  
# echo $?  
255  

would return 255 (indicating not supported) with the error.
After applying the patch, the same command now returns 0, which is incorrect. The output is as follows:

# ./perf record -o- --no-buildid --branch-filter any,save_type,u -- true  
Lowering default frequency rate from 4000 to 2000.  
Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.  
Error:  
Failure to open event 'cycles:PH' on PMU 'cpu' which will be removed.  
cycles:PH: PMU Hardware or event type doesn't support branch stack sampling.  
libperf: Miscounted nr_mmaps 0 vs 8  
WARNING: No sample_id_all support, falling back to unordered processing  
[ perf record: Woken up 1 times to write data ]  
[ perf record: Captured and wrote 0.008 MB - ]  
# echo $?  
0  

Also there were some junk result in the output which I have skipped in above result. The patch appears to alter behavior such that the unsupported or failed event open still proceeds and leading to this. 

Ian ,
Is this behaviour expected ?

Thank you,
Aditya
> 
> v2: Rebase and add tested-by tags from James Clark, Leo Yan and Atish
>    Patra who have tested on RISC-V and ARM CPUs, including the
>    problem case from before.
> 
> Ian Rogers (4):
>  perf evsel: Add pmu_name helper
>  perf stat: Fix find_stat for mixed legacy/non-legacy events
>  perf record: Skip don't fail for events that don't open
>  perf parse-events: Reapply "Prefer sysfs/JSON hardware events over
>    legacy"
> 
> tools/perf/builtin-record.c    | 22 +++++++---
> tools/perf/util/evsel.c        | 10 +++++
> tools/perf/util/evsel.h        |  1 +
> tools/perf/util/parse-events.c | 26 +++++++++---
> tools/perf/util/parse-events.l | 76 +++++++++++++++++-----------------
> tools/perf/util/parse-events.y | 60 ++++++++++++++++++---------
> tools/perf/util/pmus.c         | 20 +++++++--
> tools/perf/util/stat-shadow.c  |  3 +-
> 8 files changed, 145 insertions(+), 73 deletions(-)
> 
> -- 
> 2.47.0.277.g8800431eea-goog
> 
> 

On Wed, Nov 27, 2024 at 4:36 AM Aditya Bodkhe <Aditya.Bodkhe1@ibm.com> wrote:
>
>
>
> > On 13 Nov 2024, at 6:49 AM, Ian Rogers <irogers@google.com> wrote:
> >
> > At the RISC-V summit the topic of avoiding event data being in the
> > RISC-V PMU kernel driver came up. There is a preference for sysfs/JSON
> > events being the priority when no PMU is provided so that legacy
> > events maybe supported via json. Originally Mark Rutland also
> > expressed at LPC 2023 that doing this would resolve bugs on ARM Apple
> > M? processors, but James Clark more recently tested this and believes
> > the driver issues there may not have existed or have been resolved. In
> > any case, it is inconsistent that with a PMU event names avoid legacy
> > encodings, but when wildcarding PMUs (ie without a PMU with the event
> > name) the legacy encodings have priority.
> >
> > The patch doing this work was reverted in a v6.10 release candidate
> > as, even though the patch was posted for weeks and had been on
> > linux-next for weeks without issue, Linus was in the habit of using
> > explicit legacy events with unsupported precision options on his
> > Neoverse-N1. This machine has SLC PMU events for bus and CPU cycles
> > where ARM decided to call the events bus_cycles and cycles, the latter
> > being also a legacy event name. ARM haven't renamed the cycles event
> > to a more consistent cpu_cycles and avoided the problem. With these
> > changes the problematic event will now be skipped, a large warning
> > produced, and perf record will continue for the other PMU events. This
> > solution was proposed by Arnaldo.
> >
> > Two minor changes have been added to help with the error message and
> > to work around issues occurring with "perf stat metrics (shadow stat)
> > test".
> >
> > The patches have only been tested on my x86 non-hybrid laptop.
>
> Hi,
> After applying this patch series,we observed a regression while running the perf test suite on powerpc system. Specifically, test case for "Check branch stack sampling" now fails.
> Upon investigation, identified that patch "perf record: Skip don't fail for events that don't open"  is causing the breakage. This test case uses branch-filter as "save_type" and it is supposed to be skipped in powerpc.
> Snippet of code:
>
> skip the test if the hardware doesn't support branch stack sampling
>  and if the architecture doesn't support filter types: any,save_type,u
> if ! perf record -o- --no-buildid --branch-filter any,save_type,u -- true > /dev/null 2>&1 ; then
>     echo "skip: system doesn't support filter types: any,save_type,u"
>     exit 2
> fi
>
> Before applying the patch, running the command:
> ./perf record -o- --no-buildid --branch-filter any,save_type,u -- true
> cycles:PH: PMU Hardware or event type doesn't support branch stack sampling.
> # echo $?
> 255
>
> would return 255 (indicating not supported) with the error.
> After applying the patch, the same command now returns 0, which is incorrect. The output is as follows:
>
> # ./perf record -o- --no-buildid --branch-filter any,save_type,u -- true
> Lowering default frequency rate from 4000 to 2000.
> Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
> Error:
> Failure to open event 'cycles:PH' on PMU 'cpu' which will be removed.
> cycles:PH: PMU Hardware or event type doesn't support branch stack sampling.
> libperf: Miscounted nr_mmaps 0 vs 8
> WARNING: No sample_id_all support, falling back to unordered processing
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.008 MB - ]
> # echo $?
> 0
>
> Also there were some junk result in the output which I have skipped in above result. The patch appears to alter behavior such that the unsupported or failed event open still proceeds and leading to this.
>
> Ian ,
> Is this behaviour expected ?

Thanks Aditya, sorry for my slow response! Breaking tests isn't
expected (thanks for the report!), warning when encountering an event
that won't be recorded is expected. James Clark mentioned that we
should probably make perf record fail if it has no events to record,
my issue there was that the dummy event (for sideband things like mmap
events) is always opened and I was worried about turning "perf record
-e dummy .." into a failure. We should be able to determine if the
dummy event was created by the tool or specified the user, so I should
look to add that to the patch series and it should address this test
regression.

Thanks!
Ian