Re: [PATCH V3 4/7] driver/perf/arm_pmu_platform: Add support for BRBE attributes detection

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 10/11/22 14:51, Anshuman Khandual wrote:
> 
> On 10/10/22 19:47, James Clark wrote:
>>
>> On 06/10/2022 14:37, James Clark wrote:
>>>
>>> On 29/09/2022 08:58, Anshuman Khandual wrote:
>>>> This adds arm pmu infrastrure to probe BRBE implementation's attributes via
>>>> driver exported callbacks later. The actual BRBE feature detection will be
>>>> added by the driver itself.
>>>>
>>>> CPU specific BRBE entries, cycle count, format support gets detected during
>>>> PMU init. This information gets saved in per-cpu struct pmu_hw_events which
>>>> later helps in operating BRBE during a perf event context.
>>>>
>>>> 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>
>>>> ---
>>>>  drivers/perf/arm_pmu_platform.c | 34 +++++++++++++++++++++++++++++++++
>>>>  1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/perf/arm_pmu_platform.c b/drivers/perf/arm_pmu_platform.c
>>>> index 933b96e243b8..acdc445081aa 100644
>>>> --- a/drivers/perf/arm_pmu_platform.c
>>>> +++ b/drivers/perf/arm_pmu_platform.c
>>>> @@ -172,6 +172,36 @@ static int armpmu_request_irqs(struct arm_pmu *armpmu)
>>>>  	return err;
>>>>  }
>>>>  
>>>> +static void arm_brbe_probe_cpu(void *info)
>>>> +{
>>>> +	struct pmu_hw_events *hw_events;
>>>> +	struct arm_pmu *armpmu = info;
>>>> +
>>>> +	/*
>>>> +	 * Return from here, if BRBE driver has not been
>>>> +	 * implemented for this PMU. This helps prevent
>>>> +	 * kernel crash later when brbe_probe() will be
>>>> +	 * called on the PMU.
>>>> +	 */
>>>> +	if (!armpmu->brbe_probe)
>>>> +		return;
>>>> +
>>>> +	hw_events = per_cpu_ptr(armpmu->hw_events, smp_processor_id());
>>>> +	armpmu->brbe_probe(hw_events);
>>>> +}
>>>> +
>>>> +static int armpmu_request_brbe(struct arm_pmu *armpmu)
>>>> +{
>>>> +	int cpu, err = 0;
>>>> +
>>>> +	for_each_cpu(cpu, &armpmu->supported_cpus) {
>>>> +		err = smp_call_function_single(cpu, arm_brbe_probe_cpu, armpmu, 1);
>>> Hi Anshuman,
>>>
>>> I have LOCKDEP on and the patchset applied to perf/core (82aad7ff7) on
>>> git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git and I get
>> Can you confirm if this is currently the correct place to apply this to?
> This series applied on v6.0-rc5 after the perf ABI changes, both in kernel
> and in user space tools.
> 
>> I'm only getting 0 length branch stacks now. Seems like it could be
>> something to do with the layout of perf samples because I know that was
>> done in separate commits:
> Right, might be.
> 
>>   sudo ./perf record -j any_call -- ls
>>   ./perf report -D | grep "branch stack"
>>   ... branch stack: nr:0
>>   ... branch stack: nr:0
>>   ... branch stack: nr:0
>>   ... branch stack: nr:0
> I am planning to respin the series on 6.1-rc1 next week which should solve
> these multiple moving parts problem

There are some recent changes which require PMU driver to set data.sample_flags
indicating what kind of records are being filled in there. Here are the commits

a9a931e2666878343 ("perf: Use sample_flags for branch stack")
3aac580d5cc3001ca ("perf: Add sample_flags to indicate the PMU-filled sample data")

Following fix solves the problem for BRBE driver.

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 98e9a615d3cb..85a3aaefc0fb 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -877,6 +877,7 @@ static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu)
                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);
                }