Messages in this thread Patch in this message | | | Date | Fri, 3 Mar 2023 12:36:48 -0500 | Subject | Re: [PATCH v2 02/10] perf stat: Don't remove all grouped events when CPU maps disagree | From | "Liang, Kan" <> |
| |
On 2023-03-03 11:44 a.m., Ian Rogers wrote: > On Fri, Mar 3, 2023 at 7:50 AM Liang, Kan <kan.liang@linux.intel.com> wrote: >> >> >> >> On 2023-03-02 4:25 p.m., Ian Rogers wrote: >>> If the events in an evlist's CPU map differ then the entire group is >>> removed. For example: >>> >>> ``` >>> $ perf stat -e '{imc_free_running/data_read/,imc_free_running/data_write/,cs}' -a sleep 1 >>> WARNING: grouped events cpus do not match, disabling group: >>> anon group { imc_free_running/data_read/, imc_free_running/data_write/, cs } >>> ``` >>> >>> Change the behavior so that just the events not matching the leader >>> are removed. So in the example above, just 'cs' will be removed. >>> >>> Modify the warning so that it is produced once for each group, rather >>> than once for the entire evlist. Shrink the scope and size of the >>> warning text buffer. >> >> For the uncore, we usually have to create a group for each uncore PMU. >> The number of groups may be big. For example, on ICX, we have 40 CHA >> PMUs. For SPR, there should be more CHAs. If we have something like >> {cycles,uncore_cha/event=0x1/}, is the warning shown 40 times on ICX? >> If so, it should be very annoying. >> >> Maybe it's better to keep the current behavior which only print a >> warning once and notify the users that perf will re-group the events. >> For the details, they can get it from the -v option. > > Thanks Kan, I could imagine that but I was also worried about cases > where there are multiple groups like: > > ``` > $ perf stat -e '{imc_free_running/data_read/,cs},{uncore_clock/clockticks/,cs}' > -a sleep 1 > WARNING: grouped events cpus do not match. > Events with CPUs not matching the leader will be removed from the group. > anon group { imc_free_running/data_read/, cs } > WARNING: grouped events cpus do not match. > Events with CPUs not matching the leader will be removed from the group. > anon group { uncore_clock/clockticks/, cs } > > Performance counter stats for 'system wide': > > 1,255.75 MiB imc_free_running/data_read/ > 7,571 cs > 1,327,285,527 uncore_clock/clockticks/ > 7,571 cs > > 1.002772882 seconds time elapsed > ``` > > Knowing that both groups were broken there feels like a value add. > Given that this is a warning, and it can be fixed by moving the event > out of the group or forcing the CPUs, I lean toward being > informative/spammy as the spam is somewhat straightforwardly fixed on > the command line.
I did some tests with the patch. The issue I was worried about didn't occur. The change looks good to me.
But I found another issue. If I specify a CPU set, the group removal fails. It's not an issue of this patch. It looks like the current perf compares the user defined CPU set, rather than the PMU's cpumask.
./perf stat -e '{cycles,uncore_cha/event=0x1/}' -C0 sleep 1
Performance counter stats for 'CPU(s) 0':
<not counted> cycles <not supported> uncore_cha/event=0x1/
1.001783936 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 The events in group usually have to be from the same PMU. Try reorganizing the group.
Thanks, Kan
> > Thanks, > Ian > >> Thanks, >> Kan >>> >>> Signed-off-by: Ian Rogers <irogers@google.com> >>> --- >>> tools/perf/builtin-stat.c | 24 +++++++++++++++--------- >>> 1 file changed, 15 insertions(+), 9 deletions(-) >>> >>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c >>> index d70b1ec88594..5c12ae5efce5 100644 >>> --- a/tools/perf/builtin-stat.c >>> +++ b/tools/perf/builtin-stat.c >>> @@ -181,14 +181,13 @@ static bool cpus_map_matched(struct evsel *a, struct evsel *b) >>> >>> static void evlist__check_cpu_maps(struct evlist *evlist) >>> { >>> - struct evsel *evsel, *pos, *leader; >>> - char buf[1024]; >>> + struct evsel *evsel, *warned_leader = NULL; >>> >>> if (evlist__has_hybrid(evlist)) >>> evlist__warn_hybrid_group(evlist); >>> >>> evlist__for_each_entry(evlist, evsel) { >>> - leader = evsel__leader(evsel); >>> + struct evsel *leader = evsel__leader(evsel); >>> >>> /* Check that leader matches cpus with each member. */ >>> if (leader == evsel) >>> @@ -197,19 +196,26 @@ static void evlist__check_cpu_maps(struct evlist *evlist) >>> continue; >>> >>> /* If there's mismatch disable the group and warn user. */ >>> - WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n"); >>> - evsel__group_desc(leader, buf, sizeof(buf)); >>> - pr_warning(" %s\n", buf); >>> - >>> + if (warned_leader != leader) { >>> + char buf[200]; >>> + >>> + pr_warning("WARNING: grouped events cpus do not match.\n" >>> + "Events with CPUs not matching the leader will " >>> + "be removed from the group.\n"); >>> + evsel__group_desc(leader, buf, sizeof(buf)); >>> + pr_warning(" %s\n", buf); >>> + warned_leader = leader; >>> + } >>> if (verbose > 0) { >>> + char buf[200]; >>> + >>> cpu_map__snprint(leader->core.cpus, buf, sizeof(buf)); >>> pr_warning(" %s: %s\n", leader->name, buf); >>> cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf)); >>> pr_warning(" %s: %s\n", evsel->name, buf); >>> } >>> >>> - for_each_group_evsel(pos, leader) >>> - evsel__remove_from_group(pos, leader); >>> + evsel__remove_from_group(evsel, leader); >>> } >>> } >>>
| |