Received-SPF: pass (google.com: domain of linux-kernel+bounces-203600-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <d43d65a7-9cb9-436d-86a9-a50cbd2639d9@arm.com>
Date: Thu, 6 Jun 2024 09:27:49 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V17 0/9] arm64/perf: Enable branch stack sampling
Content-Language: en-US
To: James Clark <james.clark@arm.com>, mark.rutland@arm.com
Cc: Mark Brown <broonie@kernel.org>, Rob Herring <robh@kernel.org>,
 Marc Zyngier <maz@kernel.org>, Suzuki Poulose <suzuki.poulose@arm.com>,
 Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
 Arnaldo Carvalho de Melo <acme@kernel.org>,
 linux-perf-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-kernel@vger.kernel.org, will@kernel.org, catalin.marinas@arm.com
References: <20240405024639.1179064-1-anshuman.khandual@arm.com>
 <80d33844-bdd2-4fee-81dd-9cd37c63d42c@arm.com>
From: Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <80d33844-bdd2-4fee-81dd-9cd37c63d42c@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 5/30/24 15:17, James Clark wrote:
> 
> 
> On 05/04/2024 03:46, 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
>> the relevant register definitions could be accessed here.
>>
>> https://developer.arm.com/documentation/ddi0601/2021-12/AArch64-Registers
>>
>> This series applies on 6.9-rc2.
>>
>> Also this series is being hosted below for quick access, review and test.
>>
>> https://git.gitlab.arm.com/linux-arm/linux-anshuman.git (brbe_v17)
>>
>> There are still some open questions regarding handling multiple perf events
>> with different privilege branch filters getting on the same PMU, supporting
>> guest branch stack tracing from the host etc. Finally also looking for some
>> suggestions regarding supporting BRBE inside the guest. The series has been
>> re-organized completely as suggested earlier.
>>
>> - Anshuman
>>
> [...]
>>
>> ------------------ Possible 'branch_sample_type' Mismatch -----------------
>>
>> Branch stack sampling attributes 'event->attr.branch_sample_type' generally
>> remain the same for all the events during a perf record session.
>>
>> $perf record -e <event_1> -e <event_2> -j <branch_filters> [workload]
>>
>> event_1->attr.branch_sample_type == event_2->attr.branch_sample_type
>>
>> This 'branch_sample_type' is used to configure the BRBE hardware, when both
>> events i.e <event_1> and <event_2> get scheduled on a given PMU. But during
>> PMU HW event's privilege filter inheritance, 'branch_sample_type' does not
>> remain the same for all events. Let's consider the following example
>>
>> $perf record -e cycles:u -e instructions:k -j any,save_type ls
>>
>> cycles->attr.branch_sample_type != instructions->attr.branch_sample_type
>>
>> Because cycles event inherits PERF_SAMPLE_BRANCH_USER and instruction event
>> inherits PERF_SAMPLE_BRANCH_KERNEL. The proposed solution here configures
>> BRBE hardware with 'branch_sample_type' from last event to be added in the
>> PMU and hence captured branch records only get passed on to matching events
>> during a PMU interrupt.
>>
> 
> Hi Anshuman,
> 
> Surely because of this example we should merge? At least we have to try
> to make the most common basic command lines work. Unless we expect all
> tools to know whether the branch buffer is shared between PMUs on each
> architecture or not. The driver knows though, so can merge the settings
> because it all has to go into one BRBE.
> 
> Merging the settings in tools would be a much harder problem.

Alright, makes sense.

> 
> Any user that doesn't have permission for anything in the merged result
> can continue to get nothing.
> 
> And we can always add filtering in the kernel later on if we want to to
> make it appear to behave even more normally.

Understood.

> 
>> static int
>> armpmu_add(struct perf_event *event, int flags)
>> {
>> 	........
>> 	if (has_branch_stack(event)) {
>> 		/*
>> 		 * Reset branch records buffer if a new task event gets
>> 		 * scheduled on a PMU which might have existing records.
>> 		 * Otherwise older branch records present in the buffer
>> 		 * might leak into the new task event.
>> 		 */
>> 		if (event->ctx->task && hw_events->brbe_context != event->ctx) {
>> 			hw_events->brbe_context = event->ctx;
>> 			if (armpmu->branch_reset)
>> 				armpmu->branch_reset();
>> 		}
>> 		hw_events->brbe_users++;
>> Here ------->	hw_events->brbe_sample_type = event->attr.branch_sample_type;
>> 	}
>> 	........
>> }
>>
>> Instead of overriding existing 'branch_sample_type', both could be merged.
>>
> 
> I can't see any use case where anyone would want the override behavior.
> Especially if you imagine multiple users not even aware of each other.
> Either the current "no records for mismatches" or the merged one make sense.

Hence I had enlisted all the three available options.