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

Message ID 20231213213633.1088026-2-kan.liang@linux.intel.com
State New
Headers
Series [V3,1/2] perf top: Use evsel's cpus to replace user_requested_cpus |

Commit Message

Liang, Kan Dec. 13, 2023, 9:36 p.m. UTC
  From: Kan Liang <kan.liang@linux.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

With the patch
 $perf top
Available samples
148 cpu_atom/cycles:P/
1K cpu_core/cycles:P/

Signed-off-by: Kan Liang <kan.liang@linux.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(-)
  

Comments

Ian Rogers Dec. 13, 2023, 9:38 p.m. UTC | #1
On Wed, Dec 13, 2023 at 1:36 PM <kan.liang@linux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@linux.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
>
> With the patch
>  $perf top
> Available samples
> 148 cpu_atom/cycles:P/
> 1K cpu_core/cycles:P/
>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>
> 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
>
  
Namhyung Kim Dec. 13, 2023, 9:57 p.m. UTC | #2
On Wed, Dec 13, 2023 at 1:36 PM <kan.liang@linux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@linux.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.

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@linux.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
>
  
Liang, Kan Dec. 14, 2023, 2:44 p.m. UTC | #3
On 2023-12-13 4:57 p.m., Namhyung Kim wrote:
> On Wed, Dec 13, 2023 at 1:36 PM <kan.liang@linux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@linux.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@linux.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
>>
  

Patch

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)
 {