Re: [PATCH V3 2/2] perf top: Uniform the event name for the hybrid machine

[Liang, Kan]

On 2023-12-13 4:57 p.m., Namhyung Kim wrote:
> On Wed, Dec 13, 2023 at 1:36 PM <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>
>> It's hard to distinguish the default cycles events among hybrid PMUs.
>> For example,
>>
>>  $perf top
>> Available samples
>> 385 cycles:P
>> 903 cycles:P
>>
>> The other tool, e.g., perf record, uniforms the event name and adds the
>> hybrid PMU name before opening the event. So the events can be easily
>> distinguished. Apply the same methodology for the perf top as well.
>>
>> The record__uniquify_name() will be invoked by both record and top.
>> Move it to util/record.c
> 
> Maybe better to rename it to evlist__uniquify_name() and move it
> to util/evlist.c.

Sure, I will change it in V4.

Thanks,
Kan

> 
> Thanks,
> Namhyung
> 
>>
>> With the patch
>>  $perf top
>> Available samples
>> 148 cpu_atom/cycles:P/
>> 1K cpu_core/cycles:P/
>>
>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>> ---
>>
>> New patch to address the display concern
>> https://lore.kernel.org/lkml/e9383607-1e43-4c1a-9512-29f27784d035@xxxxxxxxxxxxxxx/
>>
>>  tools/perf/builtin-record.c | 28 +---------------------------
>>  tools/perf/builtin-top.c    |  1 +
>>  tools/perf/util/record.c    | 25 +++++++++++++++++++++++++
>>  tools/perf/util/record.h    |  2 ++
>>  4 files changed, 29 insertions(+), 27 deletions(-)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index dcf288a4fb9a..a096422a4a14 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -2216,32 +2216,6 @@ static void hit_auxtrace_snapshot_trigger(struct record *rec)
>>         }
>>  }
>>
>> -static void record__uniquify_name(struct record *rec)
>> -{
>> -       struct evsel *pos;
>> -       struct evlist *evlist = rec->evlist;
>> -       char *new_name;
>> -       int ret;
>> -
>> -       if (perf_pmus__num_core_pmus() == 1)
>> -               return;
>> -
>> -       evlist__for_each_entry(evlist, pos) {
>> -               if (!evsel__is_hybrid(pos))
>> -                       continue;
>> -
>> -               if (strchr(pos->name, '/'))
>> -                       continue;
>> -
>> -               ret = asprintf(&new_name, "%s/%s/",
>> -                              pos->pmu_name, pos->name);
>> -               if (ret) {
>> -                       free(pos->name);
>> -                       pos->name = new_name;
>> -               }
>> -       }
>> -}
>> -
>>  static int record__terminate_thread(struct record_thread *thread_data)
>>  {
>>         int err;
>> @@ -2475,7 +2449,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>>         if (data->is_pipe && rec->evlist->core.nr_entries == 1)
>>                 rec->opts.sample_id = true;
>>
>> -       record__uniquify_name(rec);
>> +       record__uniquify_name(rec->evlist);
>>
>>         /* Debug message used by test scripts */
>>         pr_debug3("perf record opening and mmapping events\n");
>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>> index cce9350177e2..4e8296654280 100644
>> --- a/tools/perf/builtin-top.c
>> +++ b/tools/perf/builtin-top.c
>> @@ -1299,6 +1299,7 @@ static int __cmd_top(struct perf_top *top)
>>                 }
>>         }
>>
>> +       record__uniquify_name(top->evlist);
>>         ret = perf_top__start_counters(top);
>>         if (ret)
>>                 return ret;
>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>> index 9eb5c6a08999..5b4be3c72cbc 100644
>> --- a/tools/perf/util/record.c
>> +++ b/tools/perf/util/record.c
>> @@ -289,3 +289,28 @@ int record__parse_freq(const struct option *opt, const char *str, int unset __ma
>>         opts->user_freq = freq;
>>         return 0;
>>  }
>> +
>> +void record__uniquify_name(struct evlist *evlist)
>> +{
>> +       struct evsel *pos;
>> +       char *new_name;
>> +       int ret;
>> +
>> +       if (perf_pmus__num_core_pmus() == 1)
>> +               return;
>> +
>> +       evlist__for_each_entry(evlist, pos) {
>> +               if (!evsel__is_hybrid(pos))
>> +                       continue;
>> +
>> +               if (strchr(pos->name, '/'))
>> +                       continue;
>> +
>> +               ret = asprintf(&new_name, "%s/%s/",
>> +                              pos->pmu_name, pos->name);
>> +               if (ret) {
>> +                       free(pos->name);
>> +                       pos->name = new_name;
>> +               }
>> +       }
>> +}
>> diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
>> index a6566134e09e..9b520ab784bc 100644
>> --- a/tools/perf/util/record.h
>> +++ b/tools/perf/util/record.h
>> @@ -8,6 +8,7 @@
>>  #include <linux/stddef.h>
>>  #include <linux/perf_event.h>
>>  #include "util/target.h"
>> +#include "util/evlist.h"
>>
>>  struct option;
>>
>> @@ -85,6 +86,7 @@ extern const char * const *record_usage;
>>  extern struct option *record_options;
>>
>>  int record__parse_freq(const struct option *opt, const char *str, int unset);
>> +void record__uniquify_name(struct evlist *evlist);
>>
>>  static inline bool record_opts__no_switch_events(const struct record_opts *opts)
>>  {
>> --
>> 2.35.1
>>