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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).