Message-ID: <0ED8731D-B183-49A5-86C3-7048D190774F@ibm.com>
Date: Wed, 27 Nov 2024 12:36:20 +0000
From: Aditya Bodkhe <Aditya.Bodkhe1@....com>
To: Ian Rogers <irogers@...gle.com>
CC: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Namhyung Kim
	<namhyung@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander
 Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
        Kan Liang
	<kan.liang@...ux.intel.com>,
        James Clark <james.clark@...aro.org>, Ze Gao
	<zegao2021@...il.com>,
        Weilin Wang <weilin.wang@...el.com>,
        Dominique
 Martinet <asmadeus@...ewreck.org>,
        Junhao He <hejunhao3@...wei.com>,
        "linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [PATCH v2 0/4] Prefer sysfs/JSON events also when no PMU is
 provided



> On 13 Nov 2024, at 6:49 AM, Ian Rogers <irogers@...gle.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
> 
> 

Powered by blists - more mailing lists