Re: [PATCH v1 21/40] perf parse-events: Wildcard legacy cache events

From: Ian Rogers
Date: Thu Apr 27 2023 - 17:02:55 EST

Next message: Marijn Suijten: "Re: [PATCH v2 08/13] drm/msm: mdss: Add SM6350 support"
Previous message: Namhyung Kim: "Re: [PATCH v1 01/40] perf stat: Introduce skippable evsels"
In reply to: Ian Rogers: "Re: [PATCH v1 21/40] perf parse-events: Wildcard legacy cache events"
Next in thread: Ian Rogers: "[PATCH v1 22/40] perf print-events: Print legacy cache events for each PMU"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Apr 26, 2023 at 10:50 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> 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.

So, following discussion with Stephane we think the right approach is
to not use a "is_event_supported" test at parse time. The event parser
should take an event name and create a perf_event_attr only. Removing
the is_event_supported test will change Intel hybrid behavior.
Wildcarded events will always try to open on both PMUs, the
expectation is that the event that failed to open will report "<not
counted>". I'll add this change in v2.

Thanks,
Ian

> > 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);

Next message: Marijn Suijten: "Re: [PATCH v2 08/13] drm/msm: mdss: Add SM6350 support"
Previous message: Namhyung Kim: "Re: [PATCH v1 01/40] perf stat: Introduce skippable evsels"
In reply to: Ian Rogers: "Re: [PATCH v1 21/40] perf parse-events: Wildcard legacy cache events"
Next in thread: Ian Rogers: "[PATCH v1 22/40] perf print-events: Print legacy cache events for each PMU"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]