Date:   Tue, 13 Sep 2022 14:12:13 +0100
From:   James Clark <james.clark@....com>
To:     Anshuman Khandual <anshuman.khandual@....com>
Cc:     Rob Herring <robh@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, peterz@...radead.org,
        acme@...nel.org, mark.rutland@....com, will@...nel.org,
        catalin.marinas@....com
Subject: Re: [PATCH V2 0/7] arm64/perf: Enable branch stack sampling



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@arm.com/
>>>
>>> https://lore.kernel.org/all/20220906084414.396220-1-anshuman.khandual@arm.com/
>>>
>>> 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.

Powered by blists - more mailing lists