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;
- 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);
--
2.40.1.495.gc816e09b53d-goog
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. 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);
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.
> 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);
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);
© 2016 - 2025 Red Hat, Inc.