Messages in this thread | | | From | Ian Rogers <> | Date | Wed, 21 Sep 2022 20:16:42 -0700 | Subject | Re: [PATCH 2/2] perf parse-events: Remove "not supported" hybrid cache events |
| |
On Wed, Sep 21, 2022 at 6:47 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> > --- > tools/perf/util/parse-events-hybrid.c | 8 +++++++- > tools/perf/util/print-events.c | 2 +- > tools/perf/util/print-events.h | 3 ++- > 3 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c > index 284f8eabd3b9..cf2e1c2e968f 100644 > --- a/tools/perf/util/parse-events-hybrid.c > +++ b/tools/perf/util/parse-events-hybrid.c > @@ -14,6 +14,7 @@ > #include "pmu.h" > #include "pmu-hybrid.h" > #include "perf.h" > +#include "print-events.h" > > static void config_hybrid_attr(struct perf_event_attr *attr, > int type, int pmu_type) > @@ -48,13 +49,18 @@ static int create_event_hybrid(__u32 config_type, int *idx, > __u64 config = attr->config; > > config_hybrid_attr(attr, config_type, pmu->type); > + > + if (attr->type == PERF_TYPE_HW_CACHE > + && !is_event_supported(attr->type, attr->config)) > + goto out;
A comment to explain this would be useful.
> + > evsel = parse_events__add_event_hybrid(list, idx, attr, name, metric_id, > pmu, config_terms); > if (evsel) > evsel->pmu_name = strdup(pmu->name); > else > return -ENOMEM;
For consistency should this use the "goto" pattern now? You can also handle the ENOMEM case for strdup.
> - > +out: > attr->type = type; > attr->config = config; > return 0; > diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c > index 04050d4f6db8..fa5cc94cfcfe 100644 > --- a/tools/perf/util/print-events.c > +++ b/tools/perf/util/print-events.c > @@ -239,7 +239,7 @@ 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 is_event_supported(u8 type, u64 config)
This makes me tempted to say this function should be in parse-events.c.
Thanks, Ian
> { > bool ret = true; > int open_return; > diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h > index 1da9910d83a6..ad2902fd0507 100644 > --- a/tools/perf/util/print-events.h > +++ b/tools/perf/util/print-events.h > @@ -1,14 +1,15 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > #ifndef __PERF_PRINT_EVENTS_H > #define __PERF_PRINT_EVENTS_H > - > #include <stdbool.h> > +#include <linux/types.h> > > struct event_symbol; > > void print_events(const char *event_glob, bool name_only, bool quiet_flag, > bool long_desc, bool details_flag, bool deprecated, > const char *pmu_name); > +bool is_event_supported(u8 type, u64 config); > int print_hwcache_events(const char *event_glob, bool name_only); > void print_sdt_events(const char *subsys_glob, const char *event_glob, > bool name_only); > -- > 2.25.1 >
| |