Messages in this thread | | | From | Ian Rogers <> | Date | Wed, 3 Apr 2024 13:26:35 -0700 | Subject | Re: [PATCH v1] perf metrics: Remove the "No_group" metric group |
| |
On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <kan.liang@linux.intel.com> wrote: > > > > On 2024-04-03 2:31 p.m., Ian Rogers wrote: > > On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <kan.liang@linux.intel.com> wrote: > >> > >> > >> > >> On 2024-04-03 12:46 p.m., Ian Rogers wrote: > >>> Rather than place metrics without a metric group in "No_group" place > >>> them in a a metric group that is their name. Still allow such metrics > >>> to be selected if "No_group" is passed, this change just impacts perf > >>> list. > >> > >> So it looks like the "No_group" is not completely removed. > >> They are just not seen in the perf list, but users can still use it via > >> perf stat -M No_group, right? > >> > >> If so, why we want to remove it from perf list? Where can the end user > >> know which metrics are included in the No_group? > >> > >> If the No_group is useless, why not completely remove it? > > > > Agreed. For command line argument deprecation we usually keep the > > option but hide it from help with PARSE_OPT_HIDDEN, so I was trying to > > follow that pattern albeit that a metric group isn't a command line > > option it's an option to an option. > > > > Perf list has a deprecated option to show the deprecated events. > The "No_group" should be a deprecated metrics group. > > If so, to follow the same pattern, I think perf list should still > display the "No_group" with the --deprecated option at least.
Such metrics would be shown twice, once under No_group and once under a metric group of their name. With deprecated events this isn't the case, you can only see them with --deprecated. Given we can see the metric without the No_group grouping, what is being added by having a No_group grouping? It feels entirely redundant and something we don't need to advertise.
Thanks, Ian
> Thanks, > Kan > > > Thanks, > > Ian > > > >> Thanks, > >> Kan > >> > >>> > >>> Signed-off-by: Ian Rogers <irogers@google.com> > >>> --- > >>> tools/perf/util/metricgroup.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > >>> index 79ef6095ab28..6ec083af14a1 100644 > >>> --- a/tools/perf/util/metricgroup.c > >>> +++ b/tools/perf/util/metricgroup.c > >>> @@ -455,7 +455,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm, > >>> const char *g; > >>> char *omg, *mg; > >>> > >>> - mg = strdup(pm->metric_group ?: "No_group"); > >>> + mg = strdup(pm->metric_group ?: pm->metric_name); > >>> if (!mg) > >>> return -ENOMEM; > >>> omg = mg; > >>> @@ -466,7 +466,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm, > >>> if (strlen(g)) > >>> me = mep_lookup(groups, g, pm->metric_name); > >>> else > >>> - me = mep_lookup(groups, "No_group", pm->metric_name); > >>> + me = mep_lookup(groups, pm->metric_name, pm->metric_name); > >>> > >>> if (me) { > >>> me->metric_desc = pm->desc;
| |