Re: [PATCH] perf arm64: Fix read PMU cpu slots

[James Clark <james.clark@arm.com>]

On 24/07/2023 17:41, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 24, 2023 at 03:40:12PM +0800, Jing Zhang escreveu:
>>
>>
>> 在 2023/7/24 下午1:06, Haixin Yu 写道:
>>> Commit f8ad6018ce3c ("perf pmu: Remove duplication around
>>>  EVENT_SOURCE_DEVICE_PATH") uses sysfs__read_ull to read a full
>>> sysfs path, which will never success. Fix it by read file directly.
>>>
>>> Signed-off-by: Haixin Yu <yuhaixin.yhx@linux.alibaba.com>
>>> ---
>>>  tools/perf/arch/arm64/util/pmu.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
>>> index 561de0cb6b95..512a8f13c4de 100644
>>> --- a/tools/perf/arch/arm64/util/pmu.c
>>> +++ b/tools/perf/arch/arm64/util/pmu.c
>>> @@ -54,10 +54,11 @@ double perf_pmu__cpu_slots_per_cycle(void)
>>>  		perf_pmu__pathname_scnprintf(path, sizeof(path),
>>>  					     pmu->name, "caps/slots");
>>>  		/*
>>> -		 * The value of slots is not greater than 32 bits, but sysfs__read_int
>>> -		 * can't read value with 0x prefix, so use sysfs__read_ull instead.
>>> +		 * The value of slots is not greater than 32 bits, but
>>> +		 * filename__read_int can't read value with 0x prefix,
>>> +		 * so use filename__read_ull instead.
>>>  		 */
>>> -		sysfs__read_ull(path, &slots);
>>> +		filename__read_ull(path, &slots);
>>>  	}
>>>  
>>>  	return slots ? (double)slots : NAN;
>>
>> Yes, the function perf_pmu__pathname_scnprintf returns the complete slots file path "/sys/bus/xxxxx/caps/slots",
>> and sysfs__read_ull will add the prefix "/sys" to the path, so the final file path becomes "/sys/sys/bus/xxxx/caps/slots"
>> which does not exist, and the slots file cannot be successfully read, so sysfs__read_ull needs to be changed to the
>> filename__read_ull.
>>
>> I tested it and it works well.
>>
>> Tested-by: Jing Zhang <renyu.zj@linux.alibaba.com>
> 
> I've applied this to my local branch, thanks.
> 
> I also added the missing:
> 
> Fixes: f8ad6018ce3c065a ("perf pmu: Remove duplication around EVENT_SOURCE_DEVICE_PATH")
> 
> This is another case where a 'perf test' entry would come in handy.
> 
> James, please check and ack,
> 
> - Arnaldo

Oops, looks like the system I tested that on doesn't report slots so it
returns NAN whether it successfully reads the file or not.

I can't think of a test that doesn't just repeat that function so I will
probably say to leave it as is (and we're not currently doing any
automated testing on any platforms that report slots either). It's quite
visible when it breaks because the topdown metrics won't work on
platforms where they should.

Sorry for the breakage!

Acked-by: James Clark <james.clark@arm.com>