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 > > Performance counter stats for 'system wide': > > 0 cpu/rf01234/ > 0 cpu/r031234/ > > The warning is reported for invalid bits. > > Signed-off-by: Jin Yao <[email protected]> > --- > 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 > + 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 (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? thanks, jirka
