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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx/
> 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