Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 5 Apr 2024 10:44:12 -0400 | Subject | Re: [PATCH v1] perf metrics: Remove the "No_group" metric group | From | "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
| ![\](/images/icornerr.gif) |