From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: zhengjun.xing@linux.intel.com, peterz@infradead.org,
mingo@redhat.com, alexander.shishkin@intel.com, jolsa@kernel.org,
namhyung@kernel.org, linux-kernel@vger.kernel.org,
linux-perf-users@vger.kernel.org, ak@linux.intel.com,
kan.liang@linux.intel.com, Yi Ammy <ammy.yi@intel.com>
Subject: Re: [PATCH v2 2/2] perf parse-events: Remove "not supported" hybrid cache events
Date: Mon, 26 Sep 2022 14:17:30 +0100 [thread overview]
Message-ID: <YzGmapYkYcpQvV/n@kernel.org> (raw)
In-Reply-To: <CAP-5=fX1VY0EqmfBJ_kVJDPy3__GDVxCOOBD5r0=ifAvJjHBPQ@mail.gmail.com>
Em Fri, Sep 23, 2022 at 09:55:16AM -0700, Ian Rogers escreveu:
> On Thu, Sep 22, 2022 at 7:58 PM <zhengjun.xing@linux.intel.com> wrote:
> >
> > From: Zhengjun Xing <zhengjun.xing@linux.intel.com>
> >
> > By default, we create two hybrid cache events, one is for cpu_core, and
> > another is for cpu_atom. But Some hybrid hardware cache events are only
> > available on one CPU PMU. For example, the 'L1-dcache-load-misses' is only
> > available on cpu_core, while the 'L1-icache-loads' is only available on
> > cpu_atom. We need to remove "not supported" hybrid cache events. By
> > extending is_event_supported() to global API and using it to check if the
> > hybrid cache events are supported before being created, we can remove the
> > "not supported" hybrid cache events.
> >
> > Before:
> >
> > # ./perf stat -e L1-dcache-load-misses,L1-icache-loads -a sleep 1
> >
> > Performance counter stats for 'system wide':
> >
> > 52,570 cpu_core/L1-dcache-load-misses/
> > <not supported> cpu_atom/L1-dcache-load-misses/
> > <not supported> cpu_core/L1-icache-loads/
> > 1,471,817 cpu_atom/L1-icache-loads/
> >
> > 1.004915229 seconds time elapsed
> >
> > After:
> >
> > # ./perf stat -e L1-dcache-load-misses,L1-icache-loads -a sleep 1
> >
> > Performance counter stats for 'system wide':
> >
> > 54,510 cpu_core/L1-dcache-load-misses/
> > 1,441,286 cpu_atom/L1-icache-loads/
> >
> > 1.005114281 seconds time elapsed
> >
> > Fixes: 30def61f64ba ("perf parse-events: Create two hybrid cache events")
> > Signed-off-by: Zhengjun Xing <zhengjun.xing@linux.intel.com>
> > Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> > Reported-by: Yi Ammy <ammy.yi@intel.com>
>
> Acked-by: Ian Rogers <irogers@google.com>
Thanks, applied.
- Arnaldo
> Thanks,
> Ian
>
> > ---
> > Change log:
> > v2:
> > * Adds a comment for removing "not supported" hybrid cache events.
> > * Remove goto and add a strdup check
> > * "is_event_supported" move to parse-events.c per Ian's suggestion.
> > * Adds Reported-by from Yi Ammy <ammy.yi@intel.com>
> >
> > tools/perf/util/parse-events-hybrid.c | 21 ++++++++++++---
> > tools/perf/util/parse-events.c | 39 +++++++++++++++++++++++++++
> > tools/perf/util/parse-events.h | 1 +
> > tools/perf/util/print-events.c | 39 ---------------------------
> > 4 files changed, 57 insertions(+), 43 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c
> > index 284f8eabd3b9..7c9f9150bad5 100644
> > --- a/tools/perf/util/parse-events-hybrid.c
> > +++ b/tools/perf/util/parse-events-hybrid.c
> > @@ -33,7 +33,8 @@ static void config_hybrid_attr(struct perf_event_attr *attr,
> > * If the PMU type ID is 0, the PERF_TYPE_RAW will be applied.
> > */
> > attr->type = type;
> > - attr->config = attr->config | ((__u64)pmu_type << PERF_PMU_TYPE_SHIFT);
> > + attr->config = (attr->config & PERF_HW_EVENT_MASK) |
> > + ((__u64)pmu_type << PERF_PMU_TYPE_SHIFT);
> > }
> >
> > static int create_event_hybrid(__u32 config_type, int *idx,
> > @@ -48,13 +49,25 @@ static int create_event_hybrid(__u32 config_type, int *idx,
> > __u64 config = attr->config;
> >
> > config_hybrid_attr(attr, config_type, pmu->type);
> > +
> > + /*
> > + * Some hybrid hardware cache events are only available on one CPU
> > + * PMU. For example, the 'L1-dcache-load-misses' is only available
> > + * on cpu_core, while the 'L1-icache-loads' is only available on
> > + * cpu_atom. We need to remove "not supported" hybrid cache events.
> > + */
> > + if (attr->type == PERF_TYPE_HW_CACHE
> > + && !is_event_supported(attr->type, attr->config))
> > + return 0;
> > +
> > evsel = parse_events__add_event_hybrid(list, idx, attr, name, metric_id,
> > pmu, config_terms);
> > - if (evsel)
> > + if (evsel) {
> > evsel->pmu_name = strdup(pmu->name);
> > - else
> > + if (!evsel->pmu_name)
> > + return -ENOMEM;
> > + } else
> > return -ENOMEM;
> > -
> > attr->type = type;
> > attr->config = config;
> > return 0;
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index f05e15acd33f..f3b2c2a87456 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -28,6 +28,7 @@
> > #include "util/parse-events-hybrid.h"
> > #include "util/pmu-hybrid.h"
> > #include "tracepoint.h"
> > +#include "thread_map.h"
> >
> > #define MAX_NAME_LEN 100
> >
> > @@ -157,6 +158,44 @@ struct event_symbol event_symbols_sw[PERF_COUNT_SW_MAX] = {
> > #define PERF_EVENT_TYPE(config) __PERF_EVENT_FIELD(config, TYPE)
> > #define PERF_EVENT_ID(config) __PERF_EVENT_FIELD(config, EVENT)
> >
> > +bool is_event_supported(u8 type, u64 config)
> > +{
> > + bool ret = true;
> > + int open_return;
> > + struct evsel *evsel;
> > + struct perf_event_attr attr = {
> > + .type = type,
> > + .config = config,
> > + .disabled = 1,
> > + };
> > + struct perf_thread_map *tmap = thread_map__new_by_tid(0);
> > +
> > + if (tmap == NULL)
> > + return false;
> > +
> > + evsel = evsel__new(&attr);
> > + if (evsel) {
> > + open_return = evsel__open(evsel, NULL, tmap);
> > + ret = open_return >= 0;
> > +
> > + if (open_return == -EACCES) {
> > + /*
> > + * This happens if the paranoid value
> > + * /proc/sys/kernel/perf_event_paranoid is set to 2
> > + * Re-run with exclude_kernel set; we don't do that
> > + * by default as some ARM machines do not support it.
> > + *
> > + */
> > + evsel->core.attr.exclude_kernel = 1;
> > + ret = evsel__open(evsel, NULL, tmap) >= 0;
> > + }
> > + evsel__delete(evsel);
> > + }
> > +
> > + perf_thread_map__put(tmap);
> > + return ret;
> > +}
> > +
> > const char *event_type(int type)
> > {
> > switch (type) {
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 7e6a601d9cd0..07df7bb7b042 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -19,6 +19,7 @@ struct option;
> > struct perf_pmu;
> >
> > bool have_tracepoints(struct list_head *evlist);
> > +bool is_event_supported(u8 type, u64 config);
> >
> > const char *event_type(int type);
> >
> > diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> > index 04050d4f6db8..c4d5d87fae2f 100644
> > --- a/tools/perf/util/print-events.c
> > +++ b/tools/perf/util/print-events.c
> > @@ -22,7 +22,6 @@
> > #include "probe-file.h"
> > #include "string2.h"
> > #include "strlist.h"
> > -#include "thread_map.h"
> > #include "tracepoint.h"
> > #include "pfm.h"
> > #include "pmu-hybrid.h"
> > @@ -239,44 +238,6 @@ void print_sdt_events(const char *subsys_glob, const char *event_glob,
> > strlist__delete(sdtlist);
> > }
> >
> > -static bool is_event_supported(u8 type, u64 config)
> > -{
> > - bool ret = true;
> > - int open_return;
> > - struct evsel *evsel;
> > - struct perf_event_attr attr = {
> > - .type = type,
> > - .config = config,
> > - .disabled = 1,
> > - };
> > - struct perf_thread_map *tmap = thread_map__new_by_tid(0);
> > -
> > - if (tmap == NULL)
> > - return false;
> > -
> > - evsel = evsel__new(&attr);
> > - if (evsel) {
> > - open_return = evsel__open(evsel, NULL, tmap);
> > - ret = open_return >= 0;
> > -
> > - if (open_return == -EACCES) {
> > - /*
> > - * This happens if the paranoid value
> > - * /proc/sys/kernel/perf_event_paranoid is set to 2
> > - * Re-run with exclude_kernel set; we don't do that
> > - * by default as some ARM machines do not support it.
> > - *
> > - */
> > - evsel->core.attr.exclude_kernel = 1;
> > - ret = evsel__open(evsel, NULL, tmap) >= 0;
> > - }
> > - evsel__delete(evsel);
> > - }
> > -
> > - perf_thread_map__put(tmap);
> > - return ret;
> > -}
> > -
> > int print_hwcache_events(const char *event_glob, bool name_only)
> > {
> > unsigned int type, op, i, evt_i = 0, evt_num = 0, npmus = 0;
> > --
> > 2.25.1
> >
--
- Arnaldo
prev parent reply other threads:[~2022-09-26 14:52 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-23 3:00 [PATCH v2 1/2] perf print-events: Fix "perf list" can not display the PMU prefix for some hybrid cache events zhengjun.xing
2022-09-23 3:00 ` [PATCH v2 2/2] perf parse-events: Remove "not supported" " zhengjun.xing
2022-09-23 16:55 ` Ian Rogers
2022-09-26 13:17 ` Arnaldo Carvalho de Melo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YzGmapYkYcpQvV/n@kernel.org \
--to=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@intel.com \
--cc=ammy.yi@intel.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=zhengjun.xing@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.