On 11/18/22 23:17, Mark Rutland wrote:
>
> Hi Anshuman,
>
> Apologies for the delayi n reviewing this.
>
> On Fri, Nov 18, 2022 at 12:09:07PM +0530, Anshuman Khandual wrote:
>> On 11/9/22 17:00, Suzuki K Poulose wrote:
>>> On 07/11/2022 06:25, Anshuman Khandual wrote:
>>>> Although BRBE is an armv8 speciifc HW feature, abstracting out its various
>>>> function callbacks at the struct arm_pmu level is preferred, as it cleaner
>>>> , easier to follow and maintain.
>>>>
>>>> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
>>>> might not fit seamlessly, when tried to be embedded via existing arm_pmu
>>>> helpers in the armv8 implementation.
>>>>
>>>> Updates the struct arm_pmu to include all required helpers that will drive
>>>> BRBE functionality for a given PMU implementation. These are the following.
>>>>
>>>> - brbe_filter    : Convert perf event filters into BRBE HW filters
>>>> - brbe_probe    : Probe BRBE HW and capture its attributes
>>>> - brbe_enable    : Enable BRBE HW with a given config
>>>> - brbe_disable    : Disable BRBE HW
>>>> - brbe_read    : Read BRBE buffer for captured branch records
>>>> - brbe_reset    : Reset BRBE buffer
>>>> - brbe_supported: Whether BRBE is supported or not
>>>>
>>>> A BRBE driver implementation needs to provide these functionalities.
>>>
>>> Could these not be hidden from the generic arm_pmu and kept in the
>>> arm64 pmu backend ? It looks like they are quite easy to simply
>>> move these to the corresponding hooks in arm64 pmu.
>>
>> We have had this discussion multiple times in the past [1], but I still
>> believe, keeping BRBE implementation hooks at the PMU level rather than
>> embedding them with other PMU events handling, is a much better logical
>> abstraction.
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>>
>> --------------------------------------------------------------------------
>>>
>>> One thing to answer in the commit msg is why we need the hooks here.
>>> Have we concluded that adding BRBE hooks to struct arm_pmu for what is
>>> an armv8 specific feature is the right approach? I don't recall
>>> reaching that conclusion.
>>
>> Although it might be possible to have this implementation embedded in
>> the existing armv8 PMU implementation, I still believe that the BRBE
>> functionalities abstracted out at the arm_pmu level with a separate
>> config option is cleaner, easier to follow and to maintain as well.
>>
>> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
>> might not fit seamlessly, when tried to be embedded via existing arm_pmu
>> helpers in the armv8 implementation.
>>
>> Nonetheless if arm_pmu based additional BRBE helpers is absolutely a no
>> go for folks here in general, will explore arm64 based implementation.
>> ----------------------------------------------------------------------------
>>
>> I am still waiting for maintainer's take on this issue. I will be happy to
>> rework this series to move all these implementation inside arm64 callbacks
>> instead, if that is required or preferred by the maintainers. But according
>> to me, this current abstraction layout is much better.
>
> To be honest, I'm not sure what's best right now; but at the moment it's not
> clear to me why this couldn't fit within the existing hooks.
>
> Above you say brbe_supported() / brbe_probe() / brbe_reset() didn't fit
> seamlessly; can you give an example of problem? I think I'm missing something
> obvious.

I tried to move them inside armv8 implementation callbacks.

arm64_pmu_brbe_supported() can be moved inside __armv8_pmuv3_map_event(), so that
event viability can be validated during armpmu_event_init(). arm64_pmu_brbe_probe()
can be moved inside __armv8pmu_probe_pmu() as you have suggested earlier on another
thread. arm64_pmu_brbe_reset() can also be moved inside armv8pmu_enable_event(),
and also armv8pmu_reset().

The only problem being armpmu_sched_task() where earlier we had BRBE reset, but I
guess it can be replaced with entire PMU reset which does the BRBE reset as well ?

static void armpmu_sched_task(struct perf_event_context *ctx, bool sched_in)
{
struct arm_pmu *armpmu = to_arm_pmu(ctx->pmu);

if (sched_in)
armpmu->reset(armpmu);
}

2022-11-29 06:47:19

by Anshuman Khandual

[permalink] [raw]

Subject: Re: [PATCH V5 5/7] arm64/perf: Drive BRBE from perf event states

On 11/18/22 23:45, Mark Rutland wrote:
> On Mon, Nov 07, 2022 at 11:55:12AM +0530, Anshuman Khandual wrote:
>> Branch stack sampling rides along the normal perf event and all the branch
>> records get captured during the PMU interrupt. This just changes perf event
>> handling on the arm64 platform to accommodate required BRBE operations that
>> will enable branch stack sampling support.
>>
>> Cc: Peter Zijlstra <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: Arnaldo Carvalho de Melo <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> arch/arm64/kernel/perf_event.c | 7 ++++++
>> drivers/perf/arm_pmu.c | 40 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 47 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index c97377e28288..97db333d1208 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -874,6 +874,13 @@ static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu)
>> if (!armpmu_event_set_period(event))
>> continue;
>>
>> + if (has_branch_stack(event)) {
>> + cpu_pmu->brbe_read(cpuc, event);
>> + data.br_stack = &cpuc->branches->brbe_stack;
>> + data.sample_flags |= PERF_SAMPLE_BRANCH_STACK;
>> + cpu_pmu->brbe_reset(cpuc);
>> + }
>> +
>> /*
>> * Perf event overflow will queue the processing of the event as
>> * an irq_work which will be taken care of in the handling of
>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
>> index 5048a500441e..1a8dca4e513e 100644
>> --- a/drivers/perf/arm_pmu.c
>> +++ b/drivers/perf/arm_pmu.c
>> @@ -271,12 +271,22 @@ armpmu_stop(struct perf_event *event, int flags)
>> {
>> struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
>> struct hw_perf_event *hwc = &event->hw;
>> + struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
>>
>> /*
>> * ARM pmu always has to update the counter, so ignore
>> * PERF_EF_UPDATE, see comments in armpmu_start().
>> */
>> if (!(hwc->state & PERF_HES_STOPPED)) {
>> + if (has_branch_stack(event)) {
>> + WARN_ON_ONCE(!hw_events->brbe_users);
>> + hw_events->brbe_users--;
>> + if (!hw_events->brbe_users) {
>> + hw_events->brbe_context = NULL;
>> + armpmu->brbe_disable(hw_events);
>> + }
>> + }
>
> Can't we do the actual enable/disable we start/stop the PMU as a whole?
>
> If we just counted the numberoof users here we could do the actual
> enable/disable in armpmu_{enable,disable}() or armv8pmu_{start,stop}(), like we
> do when checking hw_events->used_mask.

Right, it can be moved inside armv8pmu_{enable, disable}

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 7b0643fe2f13..e64832bc83ba 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -789,6 +789,19 @@ static void armv8pmu_enable_event(struct perf_event *event)
* Enable counter
*/
armv8pmu_enable_event_counter(event);
+
+ if (has_branch_stack(event)) {
+ struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+ struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+
+ if (event->ctx->task && hw_events->brbe_context != event->ctx) {
+ arm64_pmu_brbe_reset(hw_events);
+ hw_events->brbe_context = event->ctx;
+ }
+ hw_events->brbe_users++;
+ arm64_pmu_brbe_enable(event);
+ }
}

static void armv8pmu_disable_event(struct perf_event *event)
@@ -802,6 +815,14 @@ static void armv8pmu_disable_event(struct perf_event *event)
* Disable interrupt for this counter
*/
armv8pmu_disable_event_irq(event);
+
+ if (has_branch_stack(event)) {
+ struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+ struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+
+ if (!hw_events->brbe_users)
+ arm64_pmu_brbe_disable(event);
+ }
}

>
> [...]
>
>> @@ -349,6 +368,10 @@ armpmu_add(struct perf_event *event, int flags)
>> hw_events->events[idx] = event;
>>
>> hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
>> +
>> + if (has_branch_stack(event))
>> + armpmu->brbe_filter(hw_events, event);
>
> What exactly do we need to do here? Since the BRBE is shared, I'm suprised that
> there's any pwer-event configuration beyond "yes" or "no".

Entire arm64_pmu_brbe_filter() can be moved inside arm64_pmu_brbe_enable().

>
>> +
>> if (flags & PERF_EF_START)
>> armpmu_start(event, PERF_EF_RELOAD);
>>
>> @@ -443,6 +466,7 @@ __hw_perf_event_init(struct perf_event *event)
>> {
>> struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
>> struct hw_perf_event *hwc = &event->hw;
>> + struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
>> int mapping;
>>
>> hwc->flags = 0;
>> @@ -492,6 +516,9 @@ __hw_perf_event_init(struct perf_event *event)
>> local64_set(&hwc->period_left, hwc->sample_period);
>> }
>>
>> + if (has_branch_stack(event))
>> + armpmu->brbe_filter(hw_events, event);
>
> I do not understand why we would use hw_events here; at this point the event
> has only been created, and not even added yet; it doesn't have a counter index.
> isn't even being installed into HW.
>
> What am I missing?

perf event's requested branch sampling attributes need to be transferred into the
BRBE HW config, when the event gets scheduled on a given CPU. But this transfer
happens in two stages.

event -----> pmu_hw_events ----> BRBE HW

1. event->attr.branch_sample_type --> pmu_hw_events.[brbcr,brbfcr] (event add)
2. pmu_hw_events.[brbcr,brbfcr] --> BRBXXX_EL1 registers (event enable)

But I guess both these stages can be done in event_enable() and the intermediary
pmu_hw_events.[brbcr,brbfcr] elements can also be dropped.

>
>> +
>> return validate_group(event);
>> }
>>
>> @@ -520,6 +547,18 @@ static int armpmu_event_init(struct perf_event *event)
>> return __hw_perf_event_init(event);
>> }
>>
>> +static void armpmu_sched_task(struct perf_event_context *ctx, bool sched_in)
>> +{
>> + struct arm_pmu *armpmu = to_arm_pmu(ctx->pmu);
>> + struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
>> +
>> + if (!hw_events->brbe_users)
>> + return;
>> +
>> + if (sched_in)
>> + armpmu->brbe_reset(hw_events);
>
> I see that LBR does a save/restore, whereas IIUC here we discard without even
> reading the old values. Is that the intent? Shouldn't we snapshot them into the
> task context?

Yes, currently that is the intent. We cannot let the branch records from one task
slip across into another task corrupting the capture on the later. Only Intel LBR
implements such task context save/restore via hw_events, only when LBR call stack
feature is supported otherwise, all other sched_task() implementation just flush
or invalidate captured branches before changing the context.

We could implement such BRBE context store/restore mechanism via given BRBXXXINJ
registers/instructions. But this would make our context switch bit expensive and
also might not provide substantial benefit as well. Although this can be explored
later after the initial enablement.