Re: [PATCH] drivers: perf: arm_pmuv3: Update 'pmc_width' based on actual HW event width

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

On 10/10/2023 09:28, James Clark wrote:
> 
> 
> On 10/10/2023 04:03, Anshuman Khandual wrote:
>> Hi James,
>>
>> On 10/9/23 15:13, James Clark wrote:
>>>
>>>
>>> On 09/10/2023 05:37, Anshuman Khandual wrote:
>>>> This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's
>>>> width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT.
>>>>
>>>
>>> Might be worth adding why this is needed or what the actual effect is.
>>
>> To have correct 'pmc_width' visible to the user space ?
> 
> Well yeah, but for example I didn't know what that was. And it's not
> clear why it needs updating at this point in time without a link to any
> other commit or relevant section from the Arm ARM. So I had a kind of a
> "why now" question.
> 
> "To have correct 'pmc_width' visible to the user space" is definitely
> more of a what than a why.
> 

If anything this should at least have a fixes: tag on it. If you're
saying that it's now correct.

>>
>>>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>> This applies on v6.6-rc5.
>>>>
>>>>  drivers/perf/arm_pmuv3.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>>>> index fe4db1831662..94723d00548e 100644
>>>> --- a/drivers/perf/arm_pmuv3.c
>>>> +++ b/drivers/perf/arm_pmuv3.c
>>>> @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event,
>>>>  	if (userpg->cap_user_rdpmc) {
>>>>  		if (event->hw.flags & ARMPMU_EVT_64BIT)
>>>>  			userpg->pmc_width = 64;
>>>> +		else if (event->hw.flags & ARMPMU_EVT_63BIT)
>>>> +			userpg->pmc_width = 63;
>>>> +		else if (event->hw.flags & ARMPMU_EVT_47BIT)
>>>> +			userpg->pmc_width = 47;
>>>
>>> Although it doesn't explicitly say it, the bit of the docs about
>>> pmc_width in Documentation/arch/arm64/perf.rst loosely implies that this
>>> is always either 64 or 32. Now that this isn't the case it could mislead
>>> someone in userspace that they don't have to handle the now arbitrary
>>> bit widths rather than just whole bytes/ints.
>>
>> Are you suggesting that the user space would not handle pmc_width correctly
>> , once it deviates from a whole bytes/ints format ? In that case user space
>> handling might need some fixing.
>>
> 
> Not really, I'm just suggesting that anyone writing a new tool and only
> reading the docs could make that assumption. Seeing as only 32 and 64
> bit options are mentioned. So it's more to avoid misleading someone in
> the future than about fixing any existing code, as updating the docs
> wouldn't have that effect.
> 
>>>
>>> I think the fix is as simple as adding something like "the width may not
>>> match the requested value or necessarily be a multiple of 8". Unless we
>>> think this is already widely known and I suppose we could leave it as
>>> is. (The existing bit in perf that uses it already handles it correctly).
>>
>> This is from perf_event_mmap_page definition where it does not assert the
>> width to be multiple of bytes or ints. Hence the assumption should not be
>> made into the user space tools.
>>
> 
> Yeah I know its already ok for Perf which is why I mentioned it. But
> there are more tools out there than Perf, and ones that don't even exist
> yet, which people would normally read the documentation before writing.
> 
>>         /*
>>          * If cap_user_rdpmc this field provides the bit-width of the value
>>          * read using the rdpmc() or equivalent instruction. This can be used
>>          * to sign extend the result like:
>>          *
>>          *   pmc <<= 64 - width;
>>          *   pmc >>= 64 - width; // signed shift right
>>          *   count += pmc;
>>          */
>>         __u16   pmc_width;
>>
>> Moreover, on x86 too 'userpg->pmc_width' gets assigned to different values
>> although multiple of 8.
>>
>> userpg->pmc_width = x86_pmu.cntval_bits
>> arch/x86/events/amd/core.c:     .cntval_bits            = 48
>> arch/x86/events/intel/knc.c:    .cntval_bits            = 40
>> arch/x86/events/intel/p6.c:     .cntval_bits            = 32
>>
>>>
>>>>  		else
>>>>  			userpg->pmc_width = 32;
>>>>  	}
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel