[V3,4/8] perf metrics: Sort the Default metricgroup

  From: Kan Liang <kan.liang@linux.intel.com>

The new default mode will print the metrics as a metric group. The
metrics from the same metric group must be adjacent to each other in the
metric list. But the metric_list_cmp() sorts metrics by the number of
events.

Add a new sort for the Default metricgroup, which sorts by
default_metricgroup_name and metric_name.

Add is_default in the struct metric_event to indicate that it's from
the Default metricgroup.

Store the displayed metricgroup name of the Default metricgroup into
the metric expr for output.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/util/metricgroup.c | 37 +++++++++++++++++++++++++++++++++++
 tools/perf/util/metricgroup.h |  3 +++
 2 files changed, 40 insertions(+)
  

On Thu, Jun 15, 2023 at 6:54 AM <kan.liang@linux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@linux.intel.com>
>
> The new default mode will print the metrics as a metric group. The
> metrics from the same metric group must be adjacent to each other in the
> metric list. But the metric_list_cmp() sorts metrics by the number of
> events.
>
> Add a new sort for the Default metricgroup, which sorts by
> default_metricgroup_name and metric_name.
>
> Add is_default in the struct metric_event to indicate that it's from
> the Default metricgroup.
>
> Store the displayed metricgroup name of the Default metricgroup into
> the metric expr for output.
>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  tools/perf/util/metricgroup.c | 37 +++++++++++++++++++++++++++++++++++
>  tools/perf/util/metricgroup.h |  3 +++
>  2 files changed, 40 insertions(+)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 8b19644ade7d..e20adbdd5b56 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -79,6 +79,7 @@ static struct rb_node *metric_event_new(struct rblist *rblist __maybe_unused,
>                 return NULL;
>         memcpy(me, entry, sizeof(struct metric_event));
>         me->evsel = ((struct metric_event *)entry)->evsel;
> +       me->is_default = false;
>         INIT_LIST_HEAD(&me->head);
>         return &me->nd;
>  }
> @@ -1160,6 +1161,25 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
>         return right_count - left_count;
>  }
>
> +/**
> + * default_metricgroup_cmp - Implements complex key for the Default metricgroup
> + *                          that first sorts by default_metricgroup_name, then
> + *                          metric_name.
> + */
> +static int default_metricgroup_cmp(void *priv __maybe_unused,
> +                                  const struct list_head *l,
> +                                  const struct list_head *r)
> +{
> +       const struct metric *left = container_of(l, struct metric, nd);
> +       const struct metric *right = container_of(r, struct metric, nd);
> +       int diff = strcmp(right->default_metricgroup_name, left->default_metricgroup_name);
> +
> +       if (diff)
> +               return diff;
> +
> +       return strcmp(right->metric_name, left->metric_name);
> +}
> +
>  struct metricgroup__add_metric_data {
>         struct list_head *list;
>         const char *pmu;
> @@ -1515,6 +1535,7 @@ static int parse_groups(struct evlist *perf_evlist,
>         LIST_HEAD(metric_list);
>         struct metric *m;
>         bool tool_events[PERF_TOOL_MAX] = {false};
> +       bool is_default = !strcmp(str, "Default");
>         int ret;
>
>         if (metric_events_list->nr_entries == 0)
> @@ -1549,6 +1570,9 @@ static int parse_groups(struct evlist *perf_evlist,
>                         goto out;
>         }
>
> +       if (is_default)
> +               list_sort(NULL, &metric_list, default_metricgroup_cmp);
> +
>         list_for_each_entry(m, &metric_list, nd) {
>                 struct metric_event *me;
>                 struct evsel **metric_events;
> @@ -1637,6 +1661,19 @@ static int parse_groups(struct evlist *perf_evlist,
>                 expr->metric_unit = m->metric_unit;
>                 expr->metric_events = metric_events;
>                 expr->runtime = m->pctx->sctx.runtime;
> +               if (m->pmu && strcmp(m->pmu, "cpu")) {

This shouldn't compare with a PMU name like this. What happens for
memory bandwidth which could be logically with a memory controller
PMU?

> +                       char *name;
> +
> +                       if (asprintf(&name, "%s (%s)", m->default_metricgroup_name, m->pmu) < 0)
> +                               expr->default_metricgroup_name = m->default_metricgroup_name;

Who owns the string in this case? Can't you end up with
default_metricgroup_name pointing to a freed string? I think this
feels a lot more like output code, so I'm unclear why we're setting it
up in the metric.

> +                       else {
> +                               expr->default_metricgroup_name = strdup(name);

But name was just allocated, why strdup?

This is still leaking it is just the strdup now leaks and not the asprintf:
```
==2495793==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 6199 byte(s) in 340 object(s) allocated from:
   #0 0x7f175c27077b in __interceptor_strdup
../../../../src/libsanitizer/asan/asan_interceptors.cp
p:439
   #1 0x5596323b2a28 in parse_groups util/metricgroup.c:1670
   #2 0x5596323b2f08 in metricgroup__parse_groups_test util/metricgroup.c:1721
   #3 0x5596322992bf in test__parsing_callback tests/pmu-events.c:837
   #4 0x559632658ac6 in pmu_metrics_table_for_each_metric
/tmp/perf/pmu-events/pmu-events.c:61641
   #5 0x5596326590fc in pmu_for_each_core_metric
/tmp/perf/pmu-events/pmu-events.c:61742
   #6 0x559632299b7c in test__parsing tests/pmu-events.c:898
   #7 0x5596322663b0 in run_test tests/builtin-test.c:236
   #8 0x559632266655 in test_and_print tests/builtin-test.c:265
   #9 0x55963226766f in __cmd_test tests/builtin-test.c:436
   #10 0x559632268953 in cmd_test tests/builtin-test.c:559
   #11 0x5596322f5916 in run_builtin
/home/irogers/kernel.org/tools/perf/perf.c:323
   #12 0x5596322f5e87 in handle_internal_command
/home/irogers/kernel.org/tools/perf/perf.c:377
   #13 0x5596322f624f in run_argv /home/irogers/kernel.org/tools/perf/perf.c:421
   #14 0x5596322f67b7 in main /home/irogers/kernel.org/tools/perf/perf.c:537
   #15 0x7f175bebf189 in __libc_start_call_main
../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: 6199 byte(s) leaked in 340 allocation(s).
```

> +                               free(name);
> +                       }
> +               } else
> +                       expr->default_metricgroup_name = m->default_metricgroup_name;

Who owns the string in this case? Can't you end up with
default_metricgroup_name pointing to a freed string?

I spent some time trying to rationalize this to add as a patch, but
then the more I look at things like the strcmp with "cpu" my changes
were going to modify behavior in a way that would need you to test and
sign-off, so I'll hold back.

To test with leak/address sanitizer use the tmp.perf-tools-next
branch, I build with:
$ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g
-fno-omit-frame-pointer -DREFCNT_CHECKING=1 -fsanitize=address"
NO_LIBTRACEEVENT=1
but the only bit you really need is "-fsanitize=address" which both
clang and gcc support.

Address sanitizer from apt-cache is:
libasan6 - AddressSanitizer -- a fast memory error detector

Fedora and other distro.s have it too.

Thanks,
Ian

> +               if (is_default)
> +                       me->is_default = true;
>                 list_add(&expr->nd, &me->head);
>         }
>
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index bf18274c15df..d5325c6ec8e1 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -22,6 +22,7 @@ struct cgroup;
>  struct metric_event {
>         struct rb_node nd;
>         struct evsel *evsel;
> +       bool is_default; /* the metric evsel from the Default metricgroup */
>         struct list_head head; /* list of metric_expr */
>  };
>
> @@ -55,6 +56,8 @@ struct metric_expr {
>          * more human intelligible) and then add "MiB" afterward when displayed.
>          */
>         const char *metric_unit;
> +       /** Displayed metricgroup name of the Default metricgroup */
> +       const char *default_metricgroup_name;
>         /** Null terminated array of events used by the metric. */
>         struct evsel **metric_events;
>         /** Null terminated array of referenced metrics. */
> --
> 2.35.1
>
  

On 2023-06-15 6:05 p.m., Ian Rogers wrote:
> On Thu, Jun 15, 2023 at 6:54 AM <kan.liang@linux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> The new default mode will print the metrics as a metric group. The
>> metrics from the same metric group must be adjacent to each other in the
>> metric list. But the metric_list_cmp() sorts metrics by the number of
>> events.
>>
>> Add a new sort for the Default metricgroup, which sorts by
>> default_metricgroup_name and metric_name.
>>
>> Add is_default in the struct metric_event to indicate that it's from
>> the Default metricgroup.
>>
>> Store the displayed metricgroup name of the Default metricgroup into
>> the metric expr for output.
>>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>  tools/perf/util/metricgroup.c | 37 +++++++++++++++++++++++++++++++++++
>>  tools/perf/util/metricgroup.h |  3 +++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index 8b19644ade7d..e20adbdd5b56 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -79,6 +79,7 @@ static struct rb_node *metric_event_new(struct rblist *rblist __maybe_unused,
>>                 return NULL;
>>         memcpy(me, entry, sizeof(struct metric_event));
>>         me->evsel = ((struct metric_event *)entry)->evsel;
>> +       me->is_default = false;
>>         INIT_LIST_HEAD(&me->head);
>>         return &me->nd;
>>  }
>> @@ -1160,6 +1161,25 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
>>         return right_count - left_count;
>>  }
>>
>> +/**
>> + * default_metricgroup_cmp - Implements complex key for the Default metricgroup
>> + *                          that first sorts by default_metricgroup_name, then
>> + *                          metric_name.
>> + */
>> +static int default_metricgroup_cmp(void *priv __maybe_unused,
>> +                                  const struct list_head *l,
>> +                                  const struct list_head *r)
>> +{
>> +       const struct metric *left = container_of(l, struct metric, nd);
>> +       const struct metric *right = container_of(r, struct metric, nd);
>> +       int diff = strcmp(right->default_metricgroup_name, left->default_metricgroup_name);
>> +
>> +       if (diff)
>> +               return diff;
>> +
>> +       return strcmp(right->metric_name, left->metric_name);
>> +}
>> +
>>  struct metricgroup__add_metric_data {
>>         struct list_head *list;
>>         const char *pmu;
>> @@ -1515,6 +1535,7 @@ static int parse_groups(struct evlist *perf_evlist,
>>         LIST_HEAD(metric_list);
>>         struct metric *m;
>>         bool tool_events[PERF_TOOL_MAX] = {false};
>> +       bool is_default = !strcmp(str, "Default");
>>         int ret;
>>
>>         if (metric_events_list->nr_entries == 0)
>> @@ -1549,6 +1570,9 @@ static int parse_groups(struct evlist *perf_evlist,
>>                         goto out;
>>         }
>>
>> +       if (is_default)
>> +               list_sort(NULL, &metric_list, default_metricgroup_cmp);
>> +
>>         list_for_each_entry(m, &metric_list, nd) {
>>                 struct metric_event *me;
>>                 struct evsel **metric_events;
>> @@ -1637,6 +1661,19 @@ static int parse_groups(struct evlist *perf_evlist,
>>                 expr->metric_unit = m->metric_unit;
>>                 expr->metric_events = metric_events;
>>                 expr->runtime = m->pctx->sctx.runtime;
>> +               if (m->pmu && strcmp(m->pmu, "cpu")) {
> 
> This shouldn't compare with a PMU name like this. What happens for
> memory bandwidth which could be logically with a memory controller
> PMU?
> 
>> +                       char *name;
>> +
>> +                       if (asprintf(&name, "%s (%s)", m->default_metricgroup_name, m->pmu) < 0)
>> +                               expr->default_metricgroup_name = m->default_metricgroup_name;
> 
> Who owns the string in this case? Can't you end up with
> default_metricgroup_name pointing to a freed string? I think this
> feels a lot more like output code, so I'm unclear why we're setting it
> up in the metric.

Yes, we can decide when outputing the name. The below patch move it to
the output code. I will do the modification in V4.

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index acf86b15ee49..a6a5ed44a679 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1661,17 +1661,8 @@ static int parse_groups(struct evlist *perf_evlist,
 		expr->metric_unit = m->metric_unit;
 		expr->metric_events = metric_events;
 		expr->runtime = m->pctx->sctx.runtime;
-		if (m->pmu && strcmp(m->pmu, "cpu")) {
-			char *name;
-
-			if (asprintf(&name, "%s (%s)", m->default_metricgroup_name, m->pmu) < 0)
-				expr->default_metricgroup_name = m->default_metricgroup_name;
-			else
-				expr->default_metricgroup_name = name;
-		} else
-			expr->default_metricgroup_name = m->default_metricgroup_name;
-		if (is_default)
-			me->is_default = true;
+		expr->default_metricgroup_name = m->default_metricgroup_name;
+		me->is_default = is_default;
 		list_add(&expr->nd, &me->head);
 	}

diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index b25974670d30..1440b0fc7d00 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -539,6 +539,42 @@ double test_generic_metric(struct metric_expr
*mexp, int aggr_idx)
 	return ratio;
 }

+static void perf_stat__print_metricgroup_header(struct perf_stat_config
*config,
+						struct evsel *evsel,
+						void *ctxp,
+						const char *name,
+						struct perf_stat_output_ctx *out)
+{
+	bool need_full_name = perf_pmus__num_core_pmus() > 1;
+	static const char *last_name;
+	static const char *last_pmu;
+	char full_name[64];
+
+	/*
+	 * A metricgroup may have several metric events,
+	 * e.g.,TopdownL1 on e-core of ADL.
+	 * The name has been output by the first metric
+	 * event. Only align with other metics from
+	 * different metric events.
+	 */
+	if (last_name && !strcmp(last_name, name)) {
+		if (!need_full_name || !strcmp(last_pmu, evsel->pmu_name)) {
+			out->print_metricgroup_header(config, ctxp, NULL);
+			return;
+		}
+	}
+
+	if (need_full_name)
+		scnprintf(full_name, sizeof(full_name), "%s (%s)", name,
evsel->pmu_name);
+	else
+		scnprintf(full_name, sizeof(full_name), "%s", name);
+
+	out->print_metricgroup_header(config, ctxp, full_name);
+
+	last_name = name;
+	last_pmu = evsel->pmu_name;
+}
+
 /**
  * perf_stat__print_shadow_stats_metricgroup - Print out metrics
associated with the evsel
  *					       For the non-default, all metrics associated
@@ -563,7 +599,6 @@ void
*perf_stat__print_shadow_stats_metricgroup(struct perf_stat_config *config,
 	void *ctxp = out->ctx;
 	bool header_printed = false;
 	const char *name = NULL;
-	static const char *last_name;

 	me = metricgroup__lookup(metric_events, evsel, false);
 	if (me == NULL)
@@ -588,20 +623,8 @@ void
*perf_stat__print_shadow_stats_metricgroup(struct perf_stat_config *config,
 			/* Only print the name of the metricgroup once */
 			if (!header_printed) {
 				header_printed = true;
-				if (!last_name || strcmp(last_name, name)) {
-					/* Print out the name for the new metricgroup. */
-					out->print_metricgroup_header(config, ctxp, name);
-					last_name = name;
-				} else if (!strcmp(last_name, name)) {
-					/*
-					 * A metricgroup may have several metric events,
-					 * e.g.,TopdownL1 on e-core of ADL.
-					 * The name has been output by the first metric
-					 * event. Only align with other metics from
-					 * different metric events.
-					 */
-					out->print_metricgroup_header(config, ctxp, NULL);
-				}
+				perf_stat__print_metricgroup_header(config, evsel, ctxp,
+								    name, out);
 			}
 		}



> 
>> +                       else {
>> +                               expr->default_metricgroup_name = strdup(name);
> 
> But name was just allocated, why strdup?
> 
> This is still leaking it is just the strdup now leaks and not the asprintf:
> ```
> ==2495793==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 6199 byte(s) in 340 object(s) allocated from:
>    #0 0x7f175c27077b in __interceptor_strdup
> ../../../../src/libsanitizer/asan/asan_interceptors.cp
> p:439
>    #1 0x5596323b2a28 in parse_groups util/metricgroup.c:1670
>    #2 0x5596323b2f08 in metricgroup__parse_groups_test util/metricgroup.c:1721
>    #3 0x5596322992bf in test__parsing_callback tests/pmu-events.c:837
>    #4 0x559632658ac6 in pmu_metrics_table_for_each_metric
> /tmp/perf/pmu-events/pmu-events.c:61641
>    #5 0x5596326590fc in pmu_for_each_core_metric
> /tmp/perf/pmu-events/pmu-events.c:61742
>    #6 0x559632299b7c in test__parsing tests/pmu-events.c:898
>    #7 0x5596322663b0 in run_test tests/builtin-test.c:236
>    #8 0x559632266655 in test_and_print tests/builtin-test.c:265
>    #9 0x55963226766f in __cmd_test tests/builtin-test.c:436
>    #10 0x559632268953 in cmd_test tests/builtin-test.c:559
>    #11 0x5596322f5916 in run_builtin
> /home/irogers/kernel.org/tools/perf/perf.c:323
>    #12 0x5596322f5e87 in handle_internal_command
> /home/irogers/kernel.org/tools/perf/perf.c:377
>    #13 0x5596322f624f in run_argv /home/irogers/kernel.org/tools/perf/perf.c:421
>    #14 0x5596322f67b7 in main /home/irogers/kernel.org/tools/perf/perf.c:537
>    #15 0x7f175bebf189 in __libc_start_call_main
> ../sysdeps/nptl/libc_start_call_main.h:58
> 
> SUMMARY: AddressSanitizer: 6199 byte(s) leaked in 340 allocation(s).
> ```
> 
>> +                               free(name);
>> +                       }
>> +               } else
>> +                       expr->default_metricgroup_name = m->default_metricgroup_name;
> 
> Who owns the string in this case? Can't you end up with
> default_metricgroup_name pointing to a freed string?
> 
> I spent some time trying to rationalize this to add as a patch, but
> then the more I look at things like the strcmp with "cpu" my changes
> were going to modify behavior in a way that would need you to test and
> sign-off, so I'll hold back.

THe "cpu" check will be gone in v4.

> 
> To test with leak/address sanitizer use the tmp.perf-tools-next
> branch, I build with:
> $ make -C tools/perf O=/tmp/perf DEBUG=1 EXTRA_CFLAGS="-O0 -g
> -fno-omit-frame-pointer -DREFCNT_CHECKING=1 -fsanitize=address"
> NO_LIBTRACEEVENT=1
> but the only bit you really need is "-fsanitize=address" which both
> clang and gcc support.
> 
> Address sanitizer from apt-cache is:
> libasan6 - AddressSanitizer -- a fast memory error detector
> 
> Fedora and other distro.s have it too.
> 

I still cannot reproduce the memory leaks even with the above command.
I will remove the code in v4, so the memory leaks should be gone.

Thanks,
Kan

> 
>> +               if (is_default)
>> +                       me->is_default = true;
>>                 list_add(&expr->nd, &me->head);
>>         }
>>
>> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
>> index bf18274c15df..d5325c6ec8e1 100644
>> --- a/tools/perf/util/metricgroup.h
>> +++ b/tools/perf/util/metricgroup.h
>> @@ -22,6 +22,7 @@ struct cgroup;
>>  struct metric_event {
>>         struct rb_node nd;
>>         struct evsel *evsel;
>> +       bool is_default; /* the metric evsel from the Default metricgroup */
>>         struct list_head head; /* list of metric_expr */
>>  };
>>
>> @@ -55,6 +56,8 @@ struct metric_expr {
>>          * more human intelligible) and then add "MiB" afterward when displayed.
>>          */
>>         const char *metric_unit;
>> +       /** Displayed metricgroup name of the Default metricgroup */
>> +       const char *default_metricgroup_name;
>>         /** Null terminated array of events used by the metric. */
>>         struct evsel **metric_events;
>>         /** Null terminated array of referenced metrics. */
>> --
>> 2.35.1
>>