Re: [PATCH 6.12 470/826] perf stat: Uniquify event name improvements

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@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@kernel.org Signed-off-by: Ian Rogers irogers@google.com [ Fix Intel TPEBS counting mode test ] Acked-by: Kan Liang kan.liang@linux.intel.com Signed-off-by: James Clark james.clark@linaro.org Cc: Yang Jihong yangjihong@bytedance.com Cc: Dominique Martinet asmadeus@codewreck.org Cc: Colin Ian King colin.i.king@gmail.com Cc: Howard Chu howardchu95@gmail.com Cc: Ze Gao zegao2021@gmail.com Cc: Yicong Yang yangyicong@hisilicon.com Cc: Weilin Wang weilin.wang@intel.com Cc: Will Deacon will@kernel.org Cc: Mike Leach mike.leach@linaro.org Cc: Jing Zhang renyu.zj@linux.alibaba.com Cc: Yang Li yang.lee@linux.alibaba.com Cc: Leo Yan leo.yan@linux.dev Cc: ak@linux.intel.com Cc: Athira Rajeev atrajeev@linux.vnet.ibm.com Cc: linux-arm-kernel@lists.infradead.org Cc: Sun Haiyong sunhaiyong@loongson.cn Cc: John Garry john.g.garry@oracle.com Link: https://lore.kernel.org/r/20240926144851.245903-3-james.clark@linaro.org Signed-off-by: Namhyung Kim namhyung@kernel.org Signed-off-by: Sasha Levin sashal@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);