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

[Ian Rogers <irogers@google.com>]

On Wed, Apr 26, 2023 at 10:50 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Apr 26, 2023 at 3:11 AM James Clark <james.clark@arm.com> 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@google.com>
> > > ---
> > >  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);