| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Jun 2023 15:05:00 -0700
From: Ian Rogers <irogers@...gle.com>
To: kan.liang@...ux.intel.com
Cc: acme@...nel.org, mingo@...hat.com, peterz@...radead.org,
namhyung@...nel.org, jolsa@...nel.org, adrian.hunter@...el.com,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
ak@...ux.intel.com, eranian@...gle.com, ahmad.yasin@...el.com
Subject: Re: [PATCH V3 4/8] perf metrics: Sort the Default metricgroup
On Thu, Jun 15, 2023 at 6:54 AM <kan.liang@...ux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@...ux.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@...ux.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
>
Powered by blists - more mailing lists