[PATCH 6.12 470/826] perf stat: Uniquify event name improvements
James Clark
james.clark at linaro.org
Thu Dec 5 00:54:15 PST 2024
On 03/12/2024 5:24 pm, Thorsten Leemhuis wrote:
> On 03.12.24 15:43, Greg Kroah-Hartman wrote:
>> 6.12-stable review patch. If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Ian Rogers <irogers at google.com>
>>
>> [ Upstream commit 057f8bfc6f7070577523d1e3081081bbf4229c1c ]
>>
>> Without aggregation on Intel:
>
> My 6.11.y-rc and 6.12.y-rc builds for Fedora failed when building perf.
> I did not bisect, but from a brief look at the error message (see
> below) I suspect it might be caused by this patch, which is the
> second of the patch-set "Event parsing fixes":
> https://lore.kernel.org/all/20240926144851.245903-1-james.clark@linaro.org/
>
> To my untrained eyes and from a quick look I guess the first patch
> in the series needs to be backported as well:
> perf evsel: Add alternate_hw_config and use in evsel__match
> https://lore.kernel.org/all/20240926144851.245903-2-james.clark@linaro.org/
>
> This is 22a4db3c36034e ("perf evsel: Add alternate_hw_config
> and use in evsel__match") in mainline. I tried to cherry-pick it
> on-top of 6.12.2-rc1, but there were a few small conflicts.
>
> Ciao, Thorsten
>
Yes I imagine both commits are required. Another option is to not
backport this one. It looks like it's mainly to fix a test, nothing is
missing or wrong from the output, just the formatting is off.
James
> P.S.: The build error:
>
> gcc -Wp,-MD,util/.topdown.o.d -Wp,-MT,util/topdown.o -O2 -fexceptions -g -grecord-gcc-switches -pipe -Wall -Wno-complain-wrong-lang -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -mtls-dialect=gnu2 -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes -Wswitch-default -Wswitch-enum -Wundef -Wwrite-strings -Wformat -Wno-type-limits -Wstrict-aliasing=3 -Wshadow -DHAVE_SYSCALL_TABLE_SUPPORT -Iarch/x86/include/generated -DHAVE_ARCH_X86_64_SUPPORT -DHAVE_ARCH_REGS_QUERY_REGISTER_OFFSET -DNDEBUG=1 -O3 -fno-omit-frame-pointer -Wall -Wextra -std=gnu11 -fstack-protector-all -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf/util/include -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf/arch/x86/include -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/include/ -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/arch/x86/include/uapi -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/include/uapi -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/arch/x86/include/ -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/arch/x86/ -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf/util -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf -DHAVE_PTHREAD_ATTR_SETAFFINITY_NP -DHAVE_PTHREAD_BARRIER -DHAVE_EVENTFD_SUPPORT -DHAVE_GET_CURRENT_DIR_NAME -DHAVE_GETTID -DHAVE_FILE_HANDLE -DHAVE_DWARF_GETLOCATIONS_SUPPORT -DHAVE_DWARF_CFI_SUPPORT -DHAVE_AIO_SUPPORT -DHAVE_SCANDIRAT_SUPPORT -DHAVE_SCHED_GETCPU_SUPPORT -DHAVE_SETNS_SUPPORT -DHAVE_ZLIB_SUPPORT -DHAVE_LIBELF_SUPPORT -DHAVE_ELF_GETPHDRNUM_SUPPORT -DHAVE_GELF_GETNOTE_SUPPORT -DHAVE_ELF_GETSHDRSTRNDX_SUPPORT -DHAVE_DWARF_SUPPORT -DHAVE_LIBBPF_SUPPORT -DHAVE_SDT_EVENT -DHAVE_JITDUMP -DHAVE_BPF_SKEL -DHAVE_DWARF_UNWIND_SUPPORT -DHAVE_LIBCRYPTO_SUPPORT -DHAVE_SLANG_SUPPORT -DHAVE_LIBPERL_SUPPORT -DHAVE_TIMERFD_SUPPORT -DHAVE_LIBPYTHON_SUPPORT -fPIC -DHAVE_LIBLLVM_SUPPORT -I/usr/include -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -DHAVE_CXA_DEMANGLE_SUPPORT -DHAVE_LZMA_SUPPORT -DHAVE_ZSTD_SUPPORT -DHAVE_BACKTRACE_SUPPORT -DHAVE_LIBNUMA_SUPPORT -DHAVE_KVM_STAT_SUPPORT -DDISASM_FOUR_ARGS_SIGNATURE -DDISASM_INIT_STYLED -DHAVE_LIBBABELTRACE_SUPPORT -DHAVE_AUXTRACE_SUPPORT -DHAVE_JVMTI_CMLR -DHAVE_LIBTRACEEVENT -I/usr/include/traceevent -DLIBTRACEEVENT_VERSION=67067 -I/usr/include/tracefs -I/usr/include/traceevent -DLIBTRACEFS_VERSION=67065 -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf/libapi/include -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf/libsubcmd/include -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf/libsymbol/include -I/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/perf/libperf/include -D"BUILD_STR(s)=#s" -c -o util/topdown.o util/topdown.c
> util/stat-display.c: In function ‘uniquify_event_name’:
> util/stat-display.c:895:45: error: ‘struct evsel’ has no member named ‘alternate_hw_config’
> 895 | if (counter->pmu->is_core && counter->alternate_hw_config != PERF_COUNT_HW_MAX)
> | ^~
> make[4]: *** [/builddir/build/BUILD/kernel-6.12.2-build/kernel-6.12.2-rc1/linux-6.12.2-0.rc1.400.vanilla.fc41.x86_64/tools/build/Makefile.build:106: util/stat-display.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
>
>> ```
>> $ perf stat -e instructions,cycles ...
>> ```
>> Will use "cycles" for the name of the legacy cycles event but as
>> "instructions" has a sysfs name it will and a "[cpu]" PMU suffix. This
>> often breaks things as the space between the event and the PMU name
>> look like an extra column. The existing uniquify logic was also
>> uniquifying in cases when all events are core and not with uncore
>> events, it was not correctly handling modifiers, etc.
>>
>> Change the logic so that an initial pass that can disable
>> uniquification is run. For individual counters, disable uniquification
>> in more cases such as for consistency with legacy events or for
>> libpfm4 events. Don't use the "[pmu]" style suffix in uniquification,
>> always use "pmu/.../". Change how modifiers/terms are handled in the
>> uniquification so that they look like parse-able events.
>>
>> This fixes "102: perf stat metrics (shadow stat) test:" that has been
>> failing due to "instructions [cpu]" breaking its column/awk logic when
>> values aren't aggregated. This started happening when instructions
>> could match a sysfs rather than a legacy event, so the fixes tag
>> reflects this.
>>
>> Fixes: 617824a7f0f7 ("perf parse-events: Prefer sysfs/JSON hardware events over legacy")
>> Acked-by: Namhyung Kim <namhyung at kernel.org>
>> Signed-off-by: Ian Rogers <irogers at google.com>
>> [ Fix Intel TPEBS counting mode test ]
>> Acked-by: Kan Liang <kan.liang at linux.intel.com>
>> Signed-off-by: James Clark <james.clark at linaro.org>
>> Cc: Yang Jihong <yangjihong at bytedance.com>
>> Cc: Dominique Martinet <asmadeus at codewreck.org>
>> Cc: Colin Ian King <colin.i.king at gmail.com>
>> Cc: Howard Chu <howardchu95 at gmail.com>
>> Cc: Ze Gao <zegao2021 at gmail.com>
>> Cc: Yicong Yang <yangyicong at hisilicon.com>
>> Cc: Weilin Wang <weilin.wang at intel.com>
>> Cc: Will Deacon <will at kernel.org>
>> Cc: Mike Leach <mike.leach at linaro.org>
>> Cc: Jing Zhang <renyu.zj at linux.alibaba.com>
>> Cc: Yang Li <yang.lee at linux.alibaba.com>
>> Cc: Leo Yan <leo.yan at linux.dev>
>> Cc: ak at linux.intel.com
>> Cc: Athira Rajeev <atrajeev at linux.vnet.ibm.com>
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: Sun Haiyong <sunhaiyong at loongson.cn>
>> Cc: John Garry <john.g.garry at oracle.com>
>> Link: https://lore.kernel.org/r/20240926144851.245903-3-james.clark@linaro.org
>> Signed-off-by: Namhyung Kim <namhyung at kernel.org>
>> Signed-off-by: Sasha Levin <sashal at kernel.org>
>> ---
>> .../perf/tests/shell/test_stat_intel_tpebs.sh | 11 +-
>> tools/perf/util/stat-display.c | 101 ++++++++++++++----
>> 2 files changed, 85 insertions(+), 27 deletions(-)
>>
>> diff --git a/tools/perf/tests/shell/test_stat_intel_tpebs.sh b/tools/perf/tests/shell/test_stat_intel_tpebs.sh
>> index c60b29add9801..9a11f42d153ca 100755
>> --- a/tools/perf/tests/shell/test_stat_intel_tpebs.sh
>> +++ b/tools/perf/tests/shell/test_stat_intel_tpebs.sh
>> @@ -8,12 +8,15 @@ grep -q GenuineIntel /proc/cpuinfo || { echo Skipping non-Intel; exit 2; }
>> # Use this event for testing because it should exist in all platforms
>> event=cache-misses:R
>>
>> +# Hybrid platforms output like "cpu_atom/cache-misses/R", rather than as above
>> +alt_name=/cache-misses/R
>> +
>> # Without this cmd option, default value or zero is returned
>> -echo "Testing without --record-tpebs"
>> -result=$(perf stat -e "$event" true 2>&1)
>> -[[ "$result" =~ $event ]] || exit 1
>> +#echo "Testing without --record-tpebs"
>> +#result=$(perf stat -e "$event" true 2>&1)
>> +#[[ "$result" =~ $event || "$result" =~ $alt_name ]] || exit 1
>>
>> # In platforms that do not support TPEBS, it should execute without error.
>> echo "Testing with --record-tpebs"
>> result=$(perf stat -e "$event" --record-tpebs -a sleep 0.01 2>&1)
>> -[[ "$result" =~ "perf record" && "$result" =~ $event ]] || exit 1
>> +[[ "$result" =~ "perf record" && "$result" =~ $event || "$result" =~ $alt_name ]] || exit 1
>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
>> index ea96e4ebad8c8..cbff43ff8d0fb 100644
>> --- a/tools/perf/util/stat-display.c
>> +++ b/tools/perf/util/stat-display.c
>> @@ -871,38 +871,66 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
>>
>> static void uniquify_event_name(struct evsel *counter)
>> {
>> - char *new_name;
>> - char *config;
>> - int ret = 0;
>> + const char *name, *pmu_name;
>> + char *new_name, *config;
>> + int ret;
>>
>> - if (counter->uniquified_name || counter->use_config_name ||
>> - !counter->pmu_name || !strncmp(evsel__name(counter), counter->pmu_name,
>> - strlen(counter->pmu_name)))
>> + /* The evsel was already uniquified. */
>> + if (counter->uniquified_name)
>> return;
>>
>> - config = strchr(counter->name, '/');
>> + /* Avoid checking to uniquify twice. */
>> + counter->uniquified_name = true;
>> +
>> + /* The evsel has a "name=" config term or is from libpfm. */
>> + if (counter->use_config_name || counter->is_libpfm_event)
>> + return;
>> +
>> + /* Legacy no PMU event, don't uniquify. */
>> + if (!counter->pmu ||
>> + (counter->pmu->type < PERF_TYPE_MAX && counter->pmu->type != PERF_TYPE_RAW))
>> + return;
>> +
>> + /* A sysfs or json event replacing a legacy event, don't uniquify. */
>> + if (counter->pmu->is_core && counter->alternate_hw_config != PERF_COUNT_HW_MAX)
>> + return;
>> +
>> + name = evsel__name(counter);
>> + pmu_name = counter->pmu->name;
>> + /* Already prefixed by the PMU name. */
>> + if (!strncmp(name, pmu_name, strlen(pmu_name)))
>> + return;
>> +
>> + config = strchr(name, '/');
>> if (config) {
>> - if (asprintf(&new_name,
>> - "%s%s", counter->pmu_name, config) > 0) {
>> - free(counter->name);
>> - counter->name = new_name;
>> - }
>> - } else {
>> - if (evsel__is_hybrid(counter)) {
>> - ret = asprintf(&new_name, "%s/%s/",
>> - counter->pmu_name, counter->name);
>> + int len = config - name;
>> +
>> + if (config[1] == '/') {
>> + /* case: event// */
>> + ret = asprintf(&new_name, "%s/%.*s/%s", pmu_name, len, name, config + 2);
>> } else {
>> - ret = asprintf(&new_name, "%s [%s]",
>> - counter->name, counter->pmu_name);
>> + /* case: event/.../ */
>> + ret = asprintf(&new_name, "%s/%.*s,%s", pmu_name, len, name, config + 1);
>> }
>> + } else {
>> + config = strchr(name, ':');
>> + if (config) {
>> + /* case: event:.. */
>> + int len = config - name;
>>
>> - if (ret) {
>> - free(counter->name);
>> - counter->name = new_name;
>> + ret = asprintf(&new_name, "%s/%.*s/%s", pmu_name, len, name, config + 1);
>> + } else {
>> + /* case: event */
>> + ret = asprintf(&new_name, "%s/%s/", pmu_name, name);
>> }
>> }
>> -
>> - counter->uniquified_name = true;
>> + if (ret > 0) {
>> + free(counter->name);
>> + counter->name = new_name;
>> + } else {
>> + /* ENOMEM from asprintf. */
>> + counter->uniquified_name = false;
>> + }
>> }
>>
>> static bool hybrid_uniquify(struct evsel *evsel, struct perf_stat_config *config)
>> @@ -1559,6 +1587,31 @@ static void print_cgroup_counter(struct perf_stat_config *config, struct evlist
>> print_metric_end(config, os);
>> }
>>
>> +static void disable_uniquify(struct evlist *evlist)
>> +{
>> + struct evsel *counter;
>> + struct perf_pmu *last_pmu = NULL;
>> + bool first = true;
>> +
>> + evlist__for_each_entry(evlist, counter) {
>> + /* If PMUs vary then uniquify can be useful. */
>> + if (!first && counter->pmu != last_pmu)
>> + return;
>> + first = false;
>> + if (counter->pmu) {
>> + /* Allow uniquify for uncore PMUs. */
>> + if (!counter->pmu->is_core)
>> + return;
>> + /* Keep hybrid event names uniquified for clarity. */
>> + if (perf_pmus__num_core_pmus() > 1)
>> + return;
>> + }
>> + }
>> + evlist__for_each_entry_continue(evlist, counter) {
>> + counter->uniquified_name = true;
>> + }
>> +}
>> +
>> void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *config,
>> struct target *_target, struct timespec *ts,
>> int argc, const char **argv)
>> @@ -1572,6 +1625,8 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>> .first = true,
>> };
>>
>> + disable_uniquify(evlist);
>> +
>> if (config->iostat_run)
>> evlist->selected = evlist__first(evlist);
>>
>
More information about the linux-arm-kernel
mailing list