Re: [PATCH] perf pmu: Validate raw event with sysfs exported format bits
From: Jin, Yao
Date: Thu Mar 04 2021 - 23:28:48 EST
Hi Jiri,
On 3/5/2021 4:17 AM, Jiri Olsa wrote:
On Wed, Mar 03, 2021 at 01:17:36PM +0800, Jin Yao wrote:
SNIP
The set bits in 'bits' indicate the invalid bits used in config.
Finally use strbuf to report the invalid bits.
Some architectures may not export supported bits through sysfs,
so if masks is 0, perf_pmu__config_valid just returns true.
After:
Single event:
# ./perf stat -e cpu/r031234/ -a -- sleep 1
WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
Performance counter stats for 'system wide':
0 cpu/r031234/
1.001403757 seconds time elapsed
Multiple events:
# ./perf stat -e cpu/rf01234/,cpu/r031234/ -a -- sleep 1
WARNING: event config 'f01234' not valid (bits 20 22 not supported by kernel)!
WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
right, seems useful
Thanks :)
Performance counter stats for 'system wide':
0 cpu/rf01234/
0 cpu/r031234/
The warning is reported for invalid bits.
Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
---
tools/perf/util/parse-events.c | 11 ++++++++++
tools/perf/util/pmu.c | 38 ++++++++++++++++++++++++++++++++++
tools/perf/util/pmu.h | 4 ++++
3 files changed, 53 insertions(+)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 42c84adeb2fb..1820b2c0a241 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -356,6 +356,17 @@ __add_event(struct list_head *list, int *idx,
struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
if we want it just for raw/numeric, we could add it earlier on,
like to parse_events_add_numeric call
but perhaps it might be helpful to check any pmu event,
could perhaps reveal some broken format
Yes, I think so. So directly checking the attr->config here may cover more cases.
+ if (pmu && attr->type == PERF_TYPE_RAW) {
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!perf_pmu__config_valid(pmu, attr->config, &buf)) {
+ pr_warning("WARNING: event config '%llx' not valid ("
+ "bits%s not supported by kernel)!\n",
+ attr->config, buf.buf);
+ }
+ strbuf_release(&buf);
+ }
+
if (init_attr)
event_attr_init(attr);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 44ef28302fc7..5e361adb2698 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1812,3 +1812,41 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
return nr_caps;
}
+
+bool perf_pmu__config_valid(struct perf_pmu *pmu, __u64 config,
+ struct strbuf *buf)
+{
+ struct perf_pmu_format *format;
+ __u64 masks = 0, bits;
+ int i;
+
+ list_for_each_entry(format, &pmu->format, list) {
+ /*
+ * Skip extra configs such as config1/config2.
+ */
+ if (format->value > 0)
+ continue;
+
+ for_each_set_bit(i, format->bits, PERF_PMU_FORMAT_BITS)
+ masks |= 1ULL << i;
+ }
+
+ /*
+ * Kernel doesn't export any valid format bits.
+ */
+ if (masks == 0)
+ return true;
+
+ bits = config & ~masks;
+ if (bits == 0)
+ return true;
so in here you now that there's something wrong, so why
bother with the outside strbuf, when we can easily just
do all the printing in here?
For this patch, yes, I don't need to return the strbuf to caller then print outside.
Andi now comments to print the original event as well, so I need to choose #1 pass the event name to
perf_pmu__config_valid or #2 return the strbuf to caller.
+
+ for (i = 0; i < PERF_PMU_FORMAT_BITS; i++) {
+ if (bits & 0x1)
+ strbuf_addf(buf, " %d", i);
+
+ bits >>= 1;
could you use the for_each_set_bit in here?
Yes, maybe I can use the code such as :
for_each_set_bit(i, (unsigned long *) &bits, sizeof(bits) * 8)
strbuf_addf(buf, " %d", i);
Thanks
Jin Yao
thanks,
jirka