Re: [PATCH V2 0/7] arm64/perf: Enable branch stack sampling

[Anshuman Khandual]

On 9/13/22 18:42, James Clark wrote:
> 
> 
> On 13/09/2022 13:12, Anshuman Khandual wrote:
>>
>>
>> On 9/13/22 16:25, James Clark wrote:
>>>
>>> On 08/09/2022 06:10, Anshuman Khandual wrote:
>>>> This series enables perf branch stack sampling support on arm64 platform
>>>> via a new arch feature called Branch Record Buffer Extension (BRBE). All
>>>> relevant register definitions could be accessed here.
>>>>
>>>> https://developer.arm.com/documentation/ddi0601/2021-12/AArch64-Registers
>>>>
>>>> This series applies on v6.0-rc4 after the BRBE related perf ABI changes series
>>>> (V7) that was posted earlier, and a branch sample filter helper patch.
>>>>
>>>> https://lore.kernel.org/all/20220824044822.70230-1-anshuman.khandual@xxxxxxx/
>>>>
>>>> https://lore.kernel.org/all/20220906084414.396220-1-anshuman.khandual@xxxxxxx/
>>>>
>>>> Following issues have been resolved
>>>>
>>>> - Jame's concerns regarding permission inadequacy related to perfmon_capable()
>>>> - Jame's concerns regarding using perf_event_paranoid along with perfmon_capable()
>>> I don't see the resolution to this one. I'm not 100% sure of the code
>>> path used for LBR, but I think you just need to take perf_allow_kernel()
>>> into account somewhere to make this command have the same result with
>>> BRBE. Is there any contention that the permissions shouldn't behave in
>>> the same way across platforms? This is when perf_event_paranoid < 2:
>>>
>>> Intel:
>>>
>>>   $ perf record -j any -- ls
>>>
>>>   [ perf record: Woken up 1 times to write data ]
>>>   [ perf record: Captured and wrote 0.014 MB perf.data (16 samples) ]
>>>
>>> Arm:
>>>
>>>   $ perf record -j any -- ls
>>>
>>>   Error:
>>>   No permission to enable cycles event.
>>>
>> Proposed solution here just follows what we did for the SPE driver recently.
>> I would not be surprised, if there is difference in semantics in permission
>> checking across various platform perf drivers. 
> 
> SPE isn't too relevant because it's its own thing and there is no SPE
> command that can be run on other platforms. There may be something like
> perf c2c that uses SPE under the hood but if it works differently across
> platforms I would also consider that a bug and not something to be copied.
> 
>> Ideally permission should not
>> even be checked in platform drivers - either capability or perf_event_paranoid.
> 
> But it is currently. Users don't care about the code or how complicated
> the implementation is, only that the behaviour is sane. We're not
> helping Arm users or adoption of BRBE if the same command that someone
> runs somewhere else fails inexplicably, without any justification other
> than "the code didn't look right".
> 
>>
>> Unfortunately changing the permission checking framework across generic perf
>> is beyond the scope for this BRBE proposal and might be taken up later via a
> 
> Permissions are definitely not beyond the scope of this proposal because
> the code to check the permissions has been added right here:
> 
>   +		if (perfmon_capable())
>   +			event->hw.flags |= ARMPMU_EVT_PRIV;
> 
> And all it needs extra is a check of perf_allow_kernel() or similar.
> 
>> different series. Although I would be willing to accommodate any alternate
>> suggestions to improve permission checking here in the BRBE driver.
> 
> I don't think planning to change it in the future is very user friendly
> either, otherwise any help we give to people stuck will have to start
> with an explanation about how we changed the permissions model across
> versions, and their command or setup also depends on the kernel version.

I guess this discussion regarding perfmon_capable(), perf_event_paranoid,
and perf_allow_kernel() has been happening in a rather cyclical manner :)
There are multiple approaches to solve this problem both in near and long
term, and there seems to be disagreement over which is the preferred path
to be taken. Hence, will just leave the decision up to the maintainers.