[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <23a3de90-6740-4fde-9771-4f25681f6a2d@linux.intel.com>
Date: Thu, 14 Dec 2023 09:44:15 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: acme@...nel.org, irogers@...gle.com, mark.rutland@....com,
maz@...nel.org, marcan@...can.st, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH V3 2/2] perf top: Uniform the event name for the hybrid
machine
On 2023-12-13 4:57 p.m., Namhyung Kim wrote:
> On Wed, Dec 13, 2023 at 1:36 PM <kan.liang@...ux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@...ux.intel.com>
>>
>> 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@...ux.intel.com>
>> ---
>>
>> New patch to address the display concern
>> https://lore.kernel.org/lkml/e9383607-1e43-4c1a-9512-29f27784d035@linux.intel.com/
>>
>> 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
>>
Powered by blists - more mailing lists