[PATCH v1 1/2] perf stat: Fix duplicate output with --for-each-cgroup

[PATCH v1 1/2] perf stat: Fix duplicate output with --for-each-cgroup

[Namhyung Kim]

Currently it produces following output with duplicate events when
 --for-each-cgroup option is used.  It seems perf stat adds them when
it handles default events but didn't copy some fields in evsel__clone().

  $ sudo perf stat -a --for-each-cgroup / true

   Performance counter stats for 'system wide':

           8,440,165      duration_time                    /
           8,439,895      duration_time                    /
           8,440,015      duration_time                    /
           8,440,024      duration_time                    /
           8,440,075      duration_time                    /
           8,440,095      duration_time                    /
                 330      context-switches                 /             #    679.4 cs/sec  cs_per_second
              485.69 msec cpu-clock                        /             #     57.5 CPUs  CPUs_utilized
                  70      cpu-migrations                   /             #    144.1 migrations/sec  migrations_per_second
                  71      page-faults                      /             #    146.2 faults/sec  page_faults_per_second
          12,183,711      branch-misses                    /             #     10.9 %  branch_miss_rate         (5.15%)
         111,981,297      branches                         /                                                       (5.15%)
          95,844,809      branches                         /             #    197.3 M/sec  branch_frequency     (35.49%)
          65,611,429      cpu-cycles                       /             #      0.1 GHz  cycles_frequency       (98.32%)
          24,170,987      cpu-cycles                       /                                                       (95.12%)
          18,552,509      instructions                     /             #      0.8 instructions  insn_per_cycle  (95.12%)
          22,405,293      cpu-cycles                       /                                                       (64.78%)
           6,840,383      stalled-cycles-frontend          /             #     0.31 frontend_cycles_idle        (64.78%)
       <not counted>      cpu-cycles                       /
     <not supported>      stalled-cycles-backend           /             #      nan backend_cycles_idle
     <not supported>      stalled-cycles-backend           /             #      nan stalled_cycles_per_instruction
     <not supported>      instructions                     /
     <not supported>      stalled-cycles-frontend          /

         0.006546057 seconds time elapsed

  Some events weren't counted. Try disabling the NMI watchdog:
  	echo 0 > /proc/sys/kernel/nmi_watchdog
  	perf stat ...
  	echo 1 > /proc/sys/kernel/nmi_watchdog

But I'm worrying about opening same events multiple times.  Probably due
to grouping, but I'm not sure if it's beneficial in the end.  Without
duplication, it seems it won't cause multiplexing (assuming no other
users at the same time).

Fixes: a3248b5b5427d ("perf jevents: Add metric DefaultShowEvents")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d0484e4ee679e635..fefa5c73a474ff87 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -612,12 +612,18 @@ struct evsel *evsel__clone(struct evsel *orig)
 	evsel->percore = orig->percore;
 	evsel->precise_max = orig->precise_max;
 	evsel->is_libpfm_event = orig->is_libpfm_event;
+	evsel->skippable = orig->skippable;
+	evsel->retire_lat = orig->retire_lat;
+	evsel->dont_regroup = orig->dont_regroup;
 
 	evsel->exclude_GH = orig->exclude_GH;
 	evsel->sample_read = orig->sample_read;
 	evsel->collect_stat = orig->collect_stat;
 	evsel->weak_group = orig->weak_group;
 	evsel->use_config_name = orig->use_config_name;
+	evsel->default_metricgroup = orig->default_metricgroup;
+	evsel->default_show_events = orig->default_show_events;
+
 	evsel->pmu = orig->pmu;
 	evsel->first_wildcard_match = orig->first_wildcard_match;
 
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v1 2/2] perf evsel: Arrange some fields that should be cloned

[Namhyung Kim]

In the evsel, there's an internal struct to put fields need copy when
the evsel is cloned.  This is purely to make it easier track those
fields even if it sometimes failed to do so. :)

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 6b0c08958616bf04..537aca1bf0537792 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -74,6 +74,10 @@ struct evsel {
 		const char		*unit;
 		struct cgroup		*cgrp;
 		const char		*metric_id;
+
+		/* The PMU the event is from. Used for missing_features, PMU name, etc. */
+		struct perf_pmu		*pmu;
+
 		/*
 		 * This point to the first evsel with the same name, intended to store the
 		 * aggregated counts in aggregation mode.
@@ -94,6 +98,8 @@ struct evsel {
 		bool			skippable;
 		bool			retire_lat;
 		bool			dont_regroup;
+		bool			default_metricgroup; /* A member of the Default metricgroup */
+		bool			default_show_events; /* If a default group member, show the event */
 		int			bpf_fd;
 		struct bpf_object	*bpf_obj;
 		struct list_head	config_terms;
@@ -125,8 +131,6 @@ struct evsel {
 	bool			cmdline_group_boundary;
 	bool			reset_group;
 	bool			needs_auxtrace_mmap;
-	bool			default_metricgroup; /* A member of the Default metricgroup */
-	bool			default_show_events; /* If a default group member, show the event */
 	bool			needs_uniquify;
 	bool			fallenback_eacces;
 	bool			fallenback_eopnotsupp;
@@ -182,9 +186,6 @@ struct evsel {
 	unsigned long		open_flags;
 	int			precise_ip_original;
 
-	/* The PMU the event is from. Used for missing_features, PMU name, etc. */
-	struct perf_pmu		*pmu;
-
 	/* For tool events */
 	/* Beginning time subtracted when the counter is read. */
 	union {
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v1 1/2] perf stat: Fix duplicate output with --for-each-cgroup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete state cloning in `evsel__clone()` breaks TPEBS latency fallback metrics and BPF counters for duplicated events.
--

--- Patch [1]: [PATCH v1 1/2] perf stat: Fix duplicate output with --for-each-cgroup ---
commit 10359224b851cd21a00edee0e074af745faf63be
Author: Namhyung Kim <namhyung@kernel.org>

perf stat: Fix duplicate output with --for-each-cgroup

This commit fixes duplicate event output in perf stat when using the
--for-each-cgroup option. It addresses the issue by copying several
fields that were previously missed during evsel__clone().

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ea9fa04429f08..ee0b5b4c536c8 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -610,12 +610,18 @@ struct evsel *evsel__clone(struct evsel *dest, struct evsel *orig)
>  	evsel->percore = orig->percore;
>  	evsel->precise_max = orig->precise_max;
>  	evsel->is_libpfm_event = orig->is_libpfm_event;
> +	evsel->skippable = orig->skippable;
> +	evsel->retire_lat = orig->retire_lat;

[Severity: Medium]
When copying the retire_lat flag, do we also need to copy the associated
retirement_latency struct and the bpf_counter flag to avoid a regression?

Without copying the retirement_latency struct (mean, min, max), a cloned
event falling back to TPEBS might use a zeroed struct and incorrectly
calculate 0.0 latency instead of using the parsed values. 

Similarly, omitting the bpf_counter flag might cause cloned BPF events
to silently bypass BPF counting.

> +	evsel->dont_regroup = orig->dont_regroup;
>  
>  	evsel->exclude_GH = orig->exclude_GH;
>  	evsel->sample_read = orig->sample_read;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708233103.1359713-1-namhyung@kernel.org?part=1