Re: [PATCH v1 21/40] perf parse-events: Wildcard legacy cache events
From: Ian Rogers
Date: Thu Apr 27 2023 - 01:50:27 EST
On Wed, Apr 26, 2023 at 3:11 AM James Clark <james.clark@xxxxxxx> wrote:
>
>
>
> On 26/04/2023 08:00, Ian Rogers wrote:
> > It is inconsistent that "perf stat -e instructions-retired" wildcard
> > opens on all PMUs while legacy cache events like "perf stat -e
> > L1-dcache-load-miss" do not. A behavior introduced by hybrid is that a
> > legacy cache event like L1-dcache-load-miss should wildcard open on
> > all hybrid PMUs. A call to is_event_supported is necessary for each
> > PMU, a failure of which results in the event not being added. Rather
> > than special case that logic, move it into the main legacy cache event
> > case and attempt to open legacy cache events on all PMUs.
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> > tools/perf/util/parse-events-hybrid.c | 33 -------------
> > tools/perf/util/parse-events-hybrid.h | 7 ---
> > tools/perf/util/parse-events.c | 70 ++++++++++++++-------------
> > tools/perf/util/parse-events.h | 3 +-
> > tools/perf/util/parse-events.y | 2 +-
> > 5 files changed, 39 insertions(+), 76 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c
> > index 7c9f9150bad5..d2c0be051d46 100644
> > --- a/tools/perf/util/parse-events-hybrid.c
> > +++ b/tools/perf/util/parse-events-hybrid.c
> > @@ -179,36 +179,3 @@ int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
> > return add_raw_hybrid(parse_state, list, attr, name, metric_id,
> > config_terms);
> > }
> > -
> > -int parse_events__add_cache_hybrid(struct list_head *list, int *idx,
> > - struct perf_event_attr *attr,
> > - const char *name,
> > - const char *metric_id,
> > - struct list_head *config_terms,
> > - bool *hybrid,
> > - struct parse_events_state *parse_state)
> > -{
> > - struct perf_pmu *pmu;
> > - int ret;
> > -
> > - *hybrid = false;
> > - if (!perf_pmu__has_hybrid())
> > - return 0;
> > -
> > - *hybrid = true;
> > - perf_pmu__for_each_hybrid_pmu(pmu) {
> > - LIST_HEAD(terms);
> > -
> > - if (pmu_cmp(parse_state, pmu))
> > - continue;
> > -
> > - copy_config_terms(&terms, config_terms);
> > - ret = create_event_hybrid(PERF_TYPE_HW_CACHE, idx, list,
> > - attr, name, metric_id, &terms, pmu);
> > - free_config_terms(&terms);
> > - if (ret)
> > - return ret;
> > - }
> > -
> > - return 0;
> > -}
> > diff --git a/tools/perf/util/parse-events-hybrid.h b/tools/perf/util/parse-events-hybrid.h
> > index cbc05fec02a2..bc2966e73897 100644
> > --- a/tools/perf/util/parse-events-hybrid.h
> > +++ b/tools/perf/util/parse-events-hybrid.h
> > @@ -15,11 +15,4 @@ int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
> > struct list_head *config_terms,
> > bool *hybrid);
> >
> > -int parse_events__add_cache_hybrid(struct list_head *list, int *idx,
> > - struct perf_event_attr *attr,
> > - const char *name, const char *metric_id,
> > - struct list_head *config_terms,
> > - bool *hybrid,
> > - struct parse_events_state *parse_state);
> > -
> > #endif /* __PERF_PARSE_EVENTS_HYBRID_H */
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 9b2d7b6572c2..e007b2bc1ab4 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -471,46 +471,50 @@ static int parse_events__decode_legacy_cache(const char *name, int pmu_type, __u
> >
> > int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
> > struct parse_events_error *err,
> > - struct list_head *head_config,
> > - struct parse_events_state *parse_state)
> > + struct list_head *head_config)
> > {
> > - struct perf_event_attr attr;
> > - LIST_HEAD(config_terms);
> > - const char *config_name, *metric_id;
> > - int ret;
> > - bool hybrid;
> > + struct perf_pmu *pmu = NULL;
> > + bool found_supported = false;
> > + const char *config_name = get_config_name(head_config);
> > + const char *metric_id = get_config_metric_id(head_config);
> >
> > + while ((pmu = perf_pmu__scan(pmu)) != NULL) {
> > + LIST_HEAD(config_terms);
> > + struct perf_event_attr attr;
> > + int ret;
> >
> > - memset(&attr, 0, sizeof(attr));
> > - attr.type = PERF_TYPE_HW_CACHE;
> > - ret = parse_events__decode_legacy_cache(name, /*pmu_type=*/0, &attr.config);
> > - if (ret)
> > - return ret;
> > + /*
> > + * Skip uncore PMUs for performance. Software PMUs can open
> > + * PERF_TYPE_HW_CACHE, so skip.
> > + */
> > + if (pmu->is_uncore || pmu->type == PERF_TYPE_SOFTWARE)
> > + continue;
> >
> > - if (head_config) {
> > - if (config_attr(&attr, head_config, err,
> > - config_term_common))
> > - return -EINVAL;
> > + memset(&attr, 0, sizeof(attr));
> > + attr.type = PERF_TYPE_HW_CACHE;
> >
> > - if (get_config_terms(head_config, &config_terms))
> > - return -ENOMEM;
> > - }
> > + ret = parse_events__decode_legacy_cache(name, pmu->type, &attr.config);
> > + if (ret)
> > + return ret;
> >
> > - config_name = get_config_name(head_config);
> > - metric_id = get_config_metric_id(head_config);
> > - ret = parse_events__add_cache_hybrid(list, idx, &attr,
> > - config_name ? : name,
> > - metric_id,
> > - &config_terms,
> > - &hybrid, parse_state);
> > - if (hybrid)
> > - goto out_free_terms;
> > + if (!is_event_supported(PERF_TYPE_HW_CACHE, attr.config))
> > + continue;
>
> Hi Ian,
>
> I get a test failure on Arm from this commit. I think it's related to
> this check for support that's failing but I'm not sure what the
> resolution should be.
Yes, I brought in a behavior from hybrid to fail at parse time if a
legacy cache event isn't supported. The issue is the perf_event_open
may fail because of permissions and I think we probably need to
special case that and allow the parsing to succeed otherwise tests
like this will need to skip. I naively tested on a raspberry pi, which
has no metrics, and so I'll try again tomorrow on a neoverse.
> I also couldn't see why the metrics in
> test_soc/cpu/metrics.json aren't run on x86 (assuming they're generic
> 'test anywhere' type metrics?).
The testing code is split into a bunch of places for historical
reasons, but the test_soc is here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/tests/pmu-events.c?h=v6.3#n1031
'''
$ gdb --args perf test -vv -F 10
(gdb) b test__pmu_event_table
Breakpoint 1 at 0x199d7c: file tests/pmu-events.c, line 467.
(gdb) r
Starting program: /tmp/perf/perf test -vv -F 10
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
10: PMU events :
10.1: PMU event table sanity :
--- start ---
Breakpoint 1, test__pmu_event_table (test=0x5555560bd080
<suite.pmu_events>, subtest=0) at tes
ts/pmu-events.c:467
467 find_sys_events_table("pmu_events__test_soc_sys");
'''
Something I observed is that tests/parse-events.c isn't testing
against an ARM PMU and so skips a lot of testing. There should likely
be a helper so that the string in that test can be dependent on the
test platform. I worry this may expose some latent ARM issues with
things like obscure modifiers.
Thanks,
Ian
> $ perf test -vvv "parsing of PMU event table metrics with fake"
> ...
> parsing 'dcache_miss_cpi': 'l1d\-loads\-misses / inst_retired.any'
> parsing metric: l1d\-loads\-misses / inst_retired.any
> Attempting to add event pmu 'inst_retired.any' with
> 'inst_retired.any,' that may result in non-fatal errors
> After aliases, add event pmu 'inst_retired.any' with
> 'inst_retired.any,' that may result in non-fatal errors
> inst_retired.any -> fake_pmu/inst_retired.any/
> ------------------------------------------------------------
> perf_event_attr:
> type 3
> config 0x800010000
> disabled 1
> ------------------------------------------------------------
> sys_perf_event_open: pid 0 cpu -1 group_fd -1 flags 0x8
> sys_perf_event_open failed, error -2
>
> check_parse_fake failed
> test child finished with -1
> ---- end ----
> PMU events subtest 4: FAILED!
>
> >
> > - ret = add_event(list, idx, &attr, config_name ? : name, metric_id,
> > - &config_terms);
> > -out_free_terms:
> > - free_config_terms(&config_terms);
> > - return ret;
> > + found_supported = true;
> > +
> > + if (head_config) {
> > + if (config_attr(&attr, head_config, err,
> > + config_term_common))
> > + return -EINVAL;
> > +
> > + if (get_config_terms(head_config, &config_terms))
> > + return -ENOMEM;
> > + }
> > +
> > + ret = add_event(list, idx, &attr, config_name ? : name, metric_id, &config_terms);
> > + free_config_terms(&config_terms);
> > + }
> > + return found_supported ? 0: -EINVAL;
> > }
> >
> > #ifdef HAVE_LIBTRACEEVENT
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 5acb62c2e00a..0c26303f7f63 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -172,8 +172,7 @@ int parse_events_add_tool(struct parse_events_state *parse_state,
> > int tool_event);
> > int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
> > struct parse_events_error *error,
> > - struct list_head *head_config,
> > - struct parse_events_state *parse_state);
> > + struct list_head *head_config);
> > int parse_events_add_breakpoint(struct list_head *list, int *idx,
> > u64 addr, char *type, u64 len);
> > int parse_events_add_pmu(struct parse_events_state *parse_state,
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index f84fa1b132b3..cc7528558845 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -476,7 +476,7 @@ PE_LEGACY_CACHE opt_event_config
> >
> > list = alloc_list();
> > ABORT_ON(!list);
> > - err = parse_events_add_cache(list, &parse_state->idx, $1, error, $2, parse_state);
> > + err = parse_events_add_cache(list, &parse_state->idx, $1, error, $2);
> >
> > parse_events_terms__delete($2);
> > free($1);