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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <[email protected]> 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 <[email protected]>
> > ---
> > 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;
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
On Wed, Apr 3, 2024 at 11:44 AM Andi Kleen <[email protected]> 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
On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <[email protected]> wrote:
>
>
>
> On 2024-04-03 2:31 p.m., Ian Rogers wrote:
> > On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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;
On 2024-04-03 2:31 p.m., Ian Rogers wrote:
> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <[email protected]> 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 <[email protected]>
>>> ---
>>> 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;
On 2024-04-03 4:26 p.m., Ian Rogers wrote:
> On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <[email protected]> wrote:
>>
>>
>>
>> On 2024-04-03 2:31 p.m., Ian Rogers wrote:
>>> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <[email protected]> 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
On Thu, Apr 4, 2024 at 1:29 PM Liang, Kan <[email protected]> wrote:
>
>
>
> On 2024-04-03 4:26 p.m., Ian Rogers wrote:
> > On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <[email protected]> wrote:
> >>
> >>
> >>
> >> On 2024-04-03 2:31 p.m., Ian Rogers wrote:
> >>> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <[email protected]> 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/[email protected]/
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
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 <[email protected]>
Reviewed-by: Kan Liang <[email protected]>
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;
On 2024-04-04 9:16 p.m., Ian Rogers wrote:
> On Thu, Apr 4, 2024 at 1:29 PM Liang, Kan <[email protected]> wrote:
>>
>>
>>
>> On 2024-04-03 4:26 p.m., Ian Rogers wrote:
>>> On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 2024-04-03 2:31 p.m., Ian Rogers wrote:
>>>>> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <[email protected]> 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/[email protected]/
> 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
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 <[email protected]>
>
> Reviewed-by: Kan Liang <[email protected]>
Thanks, applied to perf-tools-next,
- Arnaldo