Re: [PATCH v1 21/40] perf parse-events: Wildcard legacy cache events
From: James Clark
Date: Wed Apr 26 2023 - 06:12:05 EST
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. 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?).
$ 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);