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
The valid bits (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 we use bitmap_scnprintf 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 'N/A' not valid (bits 16-17 of config '31234' not supported by kernel)!
Performance counter stats for 'system wide':
0 cpu/r031234/
1.001597373 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
0 bbb
1.001573787 seconds time elapsed
Warnings are reported for invalid bits.
Co-developed-by: Jiri Olsa <[email protected]>
Signed-off-by: Jin Yao <[email protected]>
---
tools/perf/util/parse-events.c | 3 +++
tools/perf/util/pmu.c | 36 ++++++++++++++++++++++++++++++++++
tools/perf/util/pmu.h | 3 +++
3 files changed, 42 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..03ab1e6d0418 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1812,3 +1812,39 @@ 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;
+ char buf[100];
+ 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)
+ return;
+
+ bits = config & ~masks;
+ if (bits == 0)
+ return;
+
+ bitmap_scnprintf((unsigned long *)&bits, sizeof(bits) * 8, buf, sizeof(buf));
+
+ pr_warning("WARNING: event '%s' not valid (bits %s of config "
+ "'%llx' not supported by kernel)!\n",
+ name ?: "N/A", buf, config);
+}
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 8164388478c6..160b0f561771 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -123,4 +123,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
int perf_pmu__caps_parse(struct perf_pmu *pmu);
+void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
+ char *name);
+
#endif /* __PMU_H */
--
2.17.1
On Mon, Mar 08, 2021 at 11:15:06AM +0800, Jin Yao wrote:
SNIP
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 44ef28302fc7..03ab1e6d0418 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1812,3 +1812,39 @@ 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;
> + char buf[100];
> + unsigned int i;
> +
> + list_for_each_entry(format, &pmu->format, list) {
> + /*
> + * Skip extra configs such as config1/config2.
> + */
> + if (format->value > 0)
> + continue;
sorry I did not notice before, but could you please use more direct
approach like:
if (format->value == PERF_PMU_FORMAT_VALUE_CONFIG) {
break;
}
this will be more obvious, also no need for the comment.. I spent some
time looking what's the value for ;-)
thanks,
jirka
> +
> + 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;
> +
> + bits = config & ~masks;
> + if (bits == 0)
> + return;
> +
> + bitmap_scnprintf((unsigned long *)&bits, sizeof(bits) * 8, buf, sizeof(buf));
> +
> + pr_warning("WARNING: event '%s' not valid (bits %s of config "
> + "'%llx' not supported by kernel)!\n",
> + name ?: "N/A", buf, config);
> +}
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 8164388478c6..160b0f561771 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -123,4 +123,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
>
> int perf_pmu__caps_parse(struct perf_pmu *pmu);
>
> +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
> + char *name);
> +
> #endif /* __PMU_H */
> --
> 2.17.1
>
Hi Jiri,
On 3/8/2021 6:35 PM, Jiri Olsa wrote:
> On Mon, Mar 08, 2021 at 11:15:06AM +0800, Jin Yao wrote:
>
> SNIP
>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 44ef28302fc7..03ab1e6d0418 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -1812,3 +1812,39 @@ 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;
>> + char buf[100];
>> + unsigned int i;
>> +
>> + list_for_each_entry(format, &pmu->format, list) {
>> + /*
>> + * Skip extra configs such as config1/config2.
>> + */
>> + if (format->value > 0)
>> + continue;
>
> sorry I did not notice before, but could you please use more direct
> approach like:
>
> if (format->value == PERF_PMU_FORMAT_VALUE_CONFIG) {
> break;
> }
>
> this will be more obvious, also no need for the comment.. I spent some
> time looking what's the value for ;-)
>
> thanks,
> jirka
>
Oh, yes, using PERF_PMU_FORMAT_VALUE_CONFIG is much more obvious. Sorry about that!
While it can't break the loop, because we need to iterate over the whole list to get the total valid
bits. So like:
if (format->value != PERF_PMU_FORMAT_VALUE_CONFIG)
continue;
Is it right?
I will post v4 tomorrow.
Thanks
Jin Yao
>> +
>> + 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;
>> +
>> + bits = config & ~masks;
>> + if (bits == 0)
>> + return;
>> +
>> + bitmap_scnprintf((unsigned long *)&bits, sizeof(bits) * 8, buf, sizeof(buf));
>> +
>> + pr_warning("WARNING: event '%s' not valid (bits %s of config "
>> + "'%llx' not supported by kernel)!\n",
>> + name ?: "N/A", buf, config);
>> +}
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index 8164388478c6..160b0f561771 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -123,4 +123,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
>>
>> int perf_pmu__caps_parse(struct perf_pmu *pmu);
>>
>> +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
>> + char *name);
>> +
>> #endif /* __PMU_H */
>> --
>> 2.17.1
>>
>
On Mon, Mar 08, 2021 at 08:57:49PM +0800, Jin, Yao wrote:
> Hi Jiri,
>
> On 3/8/2021 6:35 PM, Jiri Olsa wrote:
> > On Mon, Mar 08, 2021 at 11:15:06AM +0800, Jin Yao wrote:
> >
> > SNIP
> >
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index 44ef28302fc7..03ab1e6d0418 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -1812,3 +1812,39 @@ 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;
> > > + char buf[100];
> > > + unsigned int i;
> > > +
> > > + list_for_each_entry(format, &pmu->format, list) {
> > > + /*
> > > + * Skip extra configs such as config1/config2.
> > > + */
> > > + if (format->value > 0)
> > > + continue;
> >
> > sorry I did not notice before, but could you please use more direct
> > approach like:
> >
> > if (format->value == PERF_PMU_FORMAT_VALUE_CONFIG) {
> > break;
> > }
> >
> > this will be more obvious, also no need for the comment.. I spent some
> > time looking what's the value for ;-)
> >
> > thanks,
> > jirka
> >
>
> Oh, yes, using PERF_PMU_FORMAT_VALUE_CONFIG is much more obvious. Sorry about that!
>
> While it can't break the loop, because we need to iterate over the whole
> list to get the total valid bits. So like:
>
> if (format->value != PERF_PMU_FORMAT_VALUE_CONFIG)
> continue;
>
> Is it right?
sure, what I meant was to process only PERF_PMU_FORMAT_VALUE_CONFIG
and then call break, because there's no need to iterate further
jirka
Hi Jiri,
On 3/8/2021 9:14 PM, Jiri Olsa wrote:
> On Mon, Mar 08, 2021 at 08:57:49PM +0800, Jin, Yao wrote:
>> Hi Jiri,
>>
>> On 3/8/2021 6:35 PM, Jiri Olsa wrote:
>>> On Mon, Mar 08, 2021 at 11:15:06AM +0800, Jin Yao wrote:
>>>
>>> SNIP
>>>
>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>>> index 44ef28302fc7..03ab1e6d0418 100644
>>>> --- a/tools/perf/util/pmu.c
>>>> +++ b/tools/perf/util/pmu.c
>>>> @@ -1812,3 +1812,39 @@ 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;
>>>> + char buf[100];
>>>> + unsigned int i;
>>>> +
>>>> + list_for_each_entry(format, &pmu->format, list) {
>>>> + /*
>>>> + * Skip extra configs such as config1/config2.
>>>> + */
>>>> + if (format->value > 0)
>>>> + continue;
>>>
>>> sorry I did not notice before, but could you please use more direct
>>> approach like:
>>>
>>> if (format->value == PERF_PMU_FORMAT_VALUE_CONFIG) {
>>> break;
>>> }
>>>
>>> this will be more obvious, also no need for the comment.. I spent some
>>> time looking what's the value for ;-)
>>>
>>> thanks,
>>> jirka
>>>
>>
>> Oh, yes, using PERF_PMU_FORMAT_VALUE_CONFIG is much more obvious. Sorry about that!
>>
>> While it can't break the loop, because we need to iterate over the whole
>> list to get the total valid bits. So like:
>>
>> if (format->value != PERF_PMU_FORMAT_VALUE_CONFIG)
>> continue;
>>
>> Is it right?
>
> sure, what I meant was to process only PERF_PMU_FORMAT_VALUE_CONFIG
> and then call break, because there's no need to iterate further
>
> jirka
>
Sorry, maybe I still misunderstood what you suggested.
My understanding is we still need to iterate the whole formats list even we find a
PERF_PMU_FORMAT_VALUE_CONFIG.
root@kbl-ppc:/sys/devices/cpu/format# ls
any cmask edge event frontend in_tx in_tx_cp inv ldlat offcore_rsp pc umask
root@kbl-ppc:/sys/devices/cpu/format# cat any
config:21
root@kbl-ppc:/sys/devices/cpu/format# cat cmask
config:24-31
root@kbl-ppc:/sys/devices/cpu/format# cat edge
config:18
root@kbl-ppc:/sys/devices/cpu/format# cat edge
config:18
root@kbl-ppc:/sys/devices/cpu/format# cat event
config:0-7
root@kbl-ppc:/sys/devices/cpu/format# cat frontend
config1:0-23
root@kbl-ppc:/sys/devices/cpu/format# cat in_tx_cp
config:33
root@kbl-ppc:/sys/devices/cpu/format# cat inv
config:23
root@kbl-ppc:/sys/devices/cpu/format# cat ldlat
config1:0-15
root@kbl-ppc:/sys/devices/cpu/format# cat offcore_rsp
config1:0-63
root@kbl-ppc:/sys/devices/cpu/format# cat pc
config:19
root@kbl-ppc:/sys/devices/cpu/format# cat umask
config:8-15
If we break the loop when we get the first PERF_PMU_FORMAT_VALUE_CONFIG, we will only get the format
'any', right?
Thanks
Jin Yao
On Tue, Mar 09, 2021 at 11:00:02AM +0800, Jin, Yao wrote:
SNIP
> > > if (format->value != PERF_PMU_FORMAT_VALUE_CONFIG)
> > > continue;
> > >
> > > Is it right?
> >
> > sure, what I meant was to process only PERF_PMU_FORMAT_VALUE_CONFIG
> > and then call break, because there's no need to iterate further
> >
> > jirka
> >
>
> Sorry, maybe I still misunderstood what you suggested.
>
> My understanding is we still need to iterate the whole formats list even we
> find a PERF_PMU_FORMAT_VALUE_CONFIG.
>
> root@kbl-ppc:/sys/devices/cpu/format# ls
> any cmask edge event frontend in_tx in_tx_cp inv ldlat offcore_rsp pc umask
> root@kbl-ppc:/sys/devices/cpu/format# cat any
> config:21
> root@kbl-ppc:/sys/devices/cpu/format# cat cmask
> config:24-31
> root@kbl-ppc:/sys/devices/cpu/format# cat edge
> config:18
> root@kbl-ppc:/sys/devices/cpu/format# cat edge
> config:18
> root@kbl-ppc:/sys/devices/cpu/format# cat event
> config:0-7
> root@kbl-ppc:/sys/devices/cpu/format# cat frontend
> config1:0-23
> root@kbl-ppc:/sys/devices/cpu/format# cat in_tx_cp
> config:33
> root@kbl-ppc:/sys/devices/cpu/format# cat inv
> config:23
> root@kbl-ppc:/sys/devices/cpu/format# cat ldlat
> config1:0-15
> root@kbl-ppc:/sys/devices/cpu/format# cat offcore_rsp
> config1:0-63
> root@kbl-ppc:/sys/devices/cpu/format# cat pc
> config:19
> root@kbl-ppc:/sys/devices/cpu/format# cat umask
> config:8-15
>
> If we break the loop when we get the first PERF_PMU_FORMAT_VALUE_CONFIG, we
> will only get the format 'any', right?
ugh, yep.. we need all of them ;-) sry
thanks,
jirka