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

["Jin, Yao" <yao.jin@linux.intel.com>]

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