Re: [PATCH v3] perf pmu: Validate raw event with sysfs exported format bits

Jin, Yao
2021-03-09T03:01:04Z

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