On Fri, Mar 05, 2021 at 08:56:25AM +0800, Jin Yao wrote:
> A raw PMU event (eventsel+umask) in the form of rNNN is supported
> by perf but lacks of checking for the validity of raw encoding.
>
> For example, bit 16 and bit 17 are not valid on KBL but perf doesn't
> report warning when encoding with these bits.
>
> Before:
>
> # ./perf stat -e cpu/r031234/ -a -- sleep 1
>
> Performance counter stats for 'system wide':
>
> 0 cpu/r031234/
>
> 1.003798924 seconds time elapsed
>
> It may silently measure the wrong event!
>
> The kernel supported bits have been exported through
> /sys/devices/<pmu>/format/. Perf collects the information to
> 'struct perf_pmu_format' and links it to 'pmu->format' list.
>
> The 'struct perf_pmu_format' has a bitmap which records the
> valid bits for this format. For example,
>
> root@kbl-ppc:/sys/devices/cpu/format# cat umask
> config:8-15
>
> bit8-bit15 are recorded in bitmap of format 'umask'.
>
> We collect total valid bits of all formats, save to a local variable
> 'masks' and reverse it. Now '~masks' represents total invalid bits.
>
> bits = config & ~masks;
>
> 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__warn_invalid_config directly returns.
>
> After:
>
> Single event without name:
>
> # ./perf stat -e cpu/r031234/ -a -- sleep 1
> WARNING: event not valid (bits 16 17 of config '31234' not supported by
> kernel)!
>
> Performance counter stats for 'system wide':
>
> 0 cpu/r031234/
>
> 1.001414935 seconds time elapsed
>
> Multiple events with names:
>
> # ./perf stat -e cpu/rf01234,name=aaa/,cpu/r031234,name=bbb/ -a -- sleep 1
> WARNING: event 'aaa' not valid (bits 20 22 of config 'f01234' not supported
> by kernel)!
> WARNING: event 'bbb' not valid (bits 16 17 of config '31234' not supported
> by kernel)!
>
> Performance counter stats for 'system wide':
>
> 0 aaa
> 36 bbb
>
> 1.001565999 seconds time elapsed
>
> Warning is reported for invalid bits.
>
> Signed-off-by: Jin Yao <yao....@linux.intel.com>
> ---
> tools/perf/util/parse-events.c | 3 +++
> tools/perf/util/pmu.c | 46 ++++++++++++++++++++++++++++++++++
> tools/perf/util/pmu.h | 3 +++
> 3 files changed, 52 insertions(+)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 42c84adeb2fb..c0c0fab22cb8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -356,6 +356,9 @@ __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 (pmu && attr->type == PERF_TYPE_RAW)
> + perf_pmu__warn_invalid_config(pmu, attr->config, name);
> +
> if (init_attr)
> event_attr_init(attr);
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 44ef28302fc7..31e975b75766 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1812,3 +1812,49 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>
> return nr_caps;
> }
> +
> +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
> + char *name)
> +{
> + struct perf_pmu_format *format;
> + __u64 masks = 0, bits;
> + struct strbuf buf = STRBUF_INIT;
> + unsigned 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)
> + goto out;
> +
> + bits = config & ~masks;
> + if (bits == 0)
> + goto out;
> +
> + for_each_set_bit(i, (unsigned long *)&bits, sizeof(bits) * 8)
> + strbuf_addf(&buf, " %d", i);
> +
> + if (name) {
> + pr_warning("WARNING: event '%s' not valid (bits%s of config "
> + "'%llx' not supported by kernel)!\n",
> + name, buf.buf, config);
> + } else {
> + pr_warning("WARNING: event not valid (bits%s of config "
> + "'%llx' not supported by kernel)!\n",
> + buf.buf, config);
> + }
> +
> +out:
> + strbuf_release(&buf);
> +}
actualy we do have bitmap_scnprintf, which is already doing the
printing in a nice way.. we could use it like below, only compile
tested
jirka
---
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 31e975b75766..37ca5eee2667 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1818,7 +1818,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu,
__u64 config,
{
struct perf_pmu_format *format;
__u64 masks = 0, bits;
- struct strbuf buf = STRBUF_INIT;
+ char buf[50];
unsigned int i;
list_for_each_entry(format, &pmu->format, list) {
@@ -1836,25 +1836,15 @@ void perf_pmu__warn_invalid_config(struct perf_pmu
*pmu, __u64 config,
* Kernel doesn't export any valid format bits.
*/
if (masks == 0)
- goto out;
+ return;
bits = config & ~masks;
if (bits == 0)
- goto out;
-
- for_each_set_bit(i, (unsigned long *)&bits, sizeof(bits) * 8)
- strbuf_addf(&buf, " %d", i);
+ return;
- if (name) {
- pr_warning("WARNING: event '%s' not valid (bits%s of config "
- "'%llx' not supported by kernel)!\n",
- name, buf.buf, config);
- } else {
- pr_warning("WARNING: event not valid (bits%s of config "
- "'%llx' not supported by kernel)!\n",
- buf.buf, config);
- }
+ bitmap_scnprintf((unsigned long *)&bits, sizeof(bits) * 8, buf,
sizeof(buf));
-out:
- strbuf_release(&buf);
+ pr_warning("WARNING: event '%s' not valid (bits%s of config "
+ "'%llx' not supported by kernel)!\n",
+ name ?: "N/A", buf, config);
}
