Messages in this thread Patch in this message | | | From | Ian Rogers <> | Date | Fri, 3 Mar 2023 08:44:15 -0800 | Subject | Re: [PATCH v2 02/10] perf stat: Don't remove all grouped events when CPU maps disagree |
| |
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.
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); > > } > > } > >
| |