[PATCH v1] perf metrics: Remove the "No_group" metric group

[PATCH v1] perf metrics: Remove the "No_group" metric group

[Ian Rogers]

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.

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;
-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Liang, Kan]

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?

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;

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Ian Rogers]

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.

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;

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Andi Kleen]

On Wed, Apr 03, 2024 at 09:46:36AM -0700, 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.

But what's the point of it? It will just make perf list more verbose,
but I don't see any advantage.

-Andi

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Liang, Kan]

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.

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;

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Ian Rogers]

On Wed, Apr 3, 2024 at 11:44 AM Andi Kleen <ak@linux.intel.com> wrote:
>
> On Wed, Apr 03, 2024 at 09:46:36AM -0700, 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.
>
> But what's the point of it? It will just make perf list more verbose,
> but I don't see any advantage.

So it is possible to list all metrics, that's not changed here. The
thing I'm looking to change is that when a metric is standalone it
appears in "perf list metricgroups". The reason is that a metric group
can gather a bunch of related metrics, say some form of read, write
and total bandwidth, whereas something like an idle metric
("d_ratio(max(msr@tsc@ - msr@mperf@, 0), msr@tsc@)") that could get
placed in No_group is more useful if it appears in a metric group of
"idle". I'd put forward that nobody ever wants to run "idle" as part
of "No_group" whereas being able to see it as a thing in metricgroups
is useful. I want to be able to run "perf list metricgroups" and get
groups of 1 or more metrics that someone might want to pass to "perf
stat -M", currently this just shows when there is a group of more than
1 metric as there is no practice of putting a metric like "idle" into
a metric group called "idle". We could update all metrics to make it
so that when they don't have a metric group we add them to one with
their name. We could do this in jevents.py. Those changes would make
the No_group logic redundant, so we should remove it. Just updating
the No_group logic in the perf command seemed like the minimal
approach.

Thanks,
Ian

> -Andi

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Ian Rogers]

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;

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Liang, Kan]

On 2024-04-03 4:26 p.m., Ian Rogers wrote:
> 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.
You mean with the --deprecated option?
Yes, that's because the old/deprecated metrics group (No_group) is not
complete removed. So both the new name and old/deprecated name are shown
with the --deprecated option. The metrics which belong to both groups
will be shown twice.

Without the --deprecated option, only the new group and its members are
shown.

> 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.

I just want to have a generic pattern for deprecating a metrics/metrics
group that everybody can follow.

I treat the "No_group" as a normal metrics group name. So this patch is
to introduce a new name, and hide the old name. Both new and old names
can still be used.

If it's for a deprecated event, the expectation is to only see the new
name by default, and see both new name and old name with the
--deprecated option.

Now, if it's a generic deprecated metrics group, what's the expected
behavior? I prefer to follow the same pattern as a deprecated event.
If we do so, yes, there will be some redundancy with the --deprecated
option, since some members may belong to both old and new groups.
But I don't think it's an issue. It's normal that metrics belong to
different groups.

Thanks,
Kan

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Ian Rogers]

On Thu, Apr 4, 2024 at 1:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-04-03 4:26 p.m., Ian Rogers wrote:
> > 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.
> You mean with the --deprecated option?
> Yes, that's because the old/deprecated metrics group (No_group) is not
> complete removed. So both the new name and old/deprecated name are shown
> with the --deprecated option. The metrics which belong to both groups
> will be shown twice.
>
> Without the --deprecated option, only the new group and its members are
> shown.
>
> > 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.
>
> I just want to have a generic pattern for deprecating a metrics/metrics
> group that everybody can follow.

Currently there is no concept of a metric group in the json except for
descriptions. Metrics and events share the same encoding, and the
deprecated flag belongs to the event.

> I treat the "No_group" as a normal metrics group name. So this patch is
> to introduce a new name, and hide the old name. Both new and old names
> can still be used.

Why are you using No_group? I stand firm that it has no real use.

> If it's for a deprecated event, the expectation is to only see the new
> name by default, and see both new name and old name with the
> --deprecated option.
>
> Now, if it's a generic deprecated metrics group, what's the expected
> behavior? I prefer to follow the same pattern as a deprecated event.
> If we do so, yes, there will be some redundancy with the --deprecated
> option, since some members may belong to both old and new groups.
> But I don't think it's an issue. It's normal that metrics belong to
> different groups.

What you are requesting here isn't something that is unreasonable, it
is just something unconnected with this change and requires a
reorganization of the json to facilitate. As such I consider it to be
something for follow up work.

I think if we're going to restructure metric groups it would be nice
to add a more tree-like structure which could be used to visualize
metrics better. For example here:
https://lore.kernel.org/lkml/20240314055919.1979781-11-irogers@google.com/
the metrics could be shown under a tree like:
ldst
 - ldst_total
   - ldst_total_loads
 - ldst_prcnt
   - ldst_prcnt_loads
   - ldst_prcnt_stores
 - ldst_ret_lds
   - ldst_ret_lds_1
   - ldst_ret_lds_2
   - ldst_ret_lds_3
 - ldst_ret_sts
   - ldst_ret_sts_1
   - ldst_ret_sts_2
   - ldst_ret_sts_3
 - ldst_ld_hit_swpf
 - ldst_atomic_lds

but again it is follow up work to do this. In this change I just
wanted a way to list all sensibly grouped metrics or metrics in a
group just on their own which doesn't require some kind of analysis of
metric groups. No_group has no use so let's just get rid of it.

Thanks,
Ian

> Thanks,
> Kan

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Liang, Kan]

On 2024-04-04 9:16 p.m., Ian Rogers wrote:
> On Thu, Apr 4, 2024 at 1:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2024-04-03 4:26 p.m., Ian Rogers wrote:
>>> 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.
>> You mean with the --deprecated option?
>> Yes, that's because the old/deprecated metrics group (No_group) is not
>> complete removed. So both the new name and old/deprecated name are shown
>> with the --deprecated option. The metrics which belong to both groups
>> will be shown twice.
>>
>> Without the --deprecated option, only the new group and its members are
>> shown.
>>
>>> 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.
>>
>> I just want to have a generic pattern for deprecating a metrics/metrics
>> group that everybody can follow.
> 
> Currently there is no concept of a metric group in the json except for
> descriptions. Metrics and events share the same encoding, and the
> deprecated flag belongs to the event.
> 
>> I treat the "No_group" as a normal metrics group name. So this patch is
>> to introduce a new name, and hide the old name. Both new and old names
>> can still be used.
> 
> Why are you using No_group? I stand firm that it has no real use.
> 
>> If it's for a deprecated event, the expectation is to only see the new
>> name by default, and see both new name and old name with the
>> --deprecated option.
>>
>> Now, if it's a generic deprecated metrics group, what's the expected
>> behavior? I prefer to follow the same pattern as a deprecated event.
>> If we do so, yes, there will be some redundancy with the --deprecated
>> option, since some members may belong to both old and new groups.
>> But I don't think it's an issue. It's normal that metrics belong to
>> different groups.
> 
> What you are requesting here isn't something that is unreasonable, it
> is just something unconnected with this change and requires a
> reorganization of the json to facilitate. As such I consider it to be
> something for follow up work.
> 
> I think if we're going to restructure metric groups it would be nice
> to add a more tree-like structure which could be used to visualize
> metrics better. For example here:
> https://lore.kernel.org/lkml/20240314055919.1979781-11-irogers@google.com/
> the metrics could be shown under a tree like:
> ldst
>  - ldst_total
>    - ldst_total_loads
>  - ldst_prcnt
>    - ldst_prcnt_loads
>    - ldst_prcnt_stores
>  - ldst_ret_lds
>    - ldst_ret_lds_1
>    - ldst_ret_lds_2
>    - ldst_ret_lds_3
>  - ldst_ret_sts
>    - ldst_ret_sts_1
>    - ldst_ret_sts_2
>    - ldst_ret_sts_3
>  - ldst_ld_hit_swpf
>  - ldst_atomic_lds
>

Yes, a tree-like output looks much better.


> but again it is follow up work to do this. In this change I just
> wanted a way to list all sensibly grouped metrics or metrics in a
> group just on their own which doesn't require some kind of analysis of
> metric groups. No_group has no use so let's just get rid of it.
> 

I agree that there should be no one to use the No_group. Just hide it
should be fine. Maybe we can have further discussion when someday we try
to deprecate a meaningful metrics/metrics group.

Thanks,
Kan

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Liang, Kan]

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.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> ---
>  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;

Re: [PATCH v1] perf metrics: Remove the "No_group" metric group

[Arnaldo Carvalho de Melo]

On Fri, Apr 05, 2024 at 10:45:59AM -0400, Liang, Kan 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.
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks, applied to perf-tools-next,

- Arnaldo