lkml.org 
[lkml]   [2022]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC V1 06/11] arm64/perf: Drive BRBE from perf event states
On Thu, Jan 27, 2022 at 6:20 AM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
>
> On 1/26/22 10:37 PM, Rob Herring wrote:
> > On Mon, Jan 24, 2022 at 10:00:48AM +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 <peterz@infradead.org>
> >> Cc: Ingo Molnar <mingo@redhat.com>
> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: linux-perf-users@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >> ---
> >> arch/arm64/kernel/perf_event.c | 6 +++++
> >> drivers/perf/arm_pmu.c | 40 ++++++++++++++++++++++++++++++++++
> >> 2 files changed, 46 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> >> index f6a47036b0b4..11c82c8f2eec 100644
> >> --- a/arch/arm64/kernel/perf_event.c
> >> +++ b/arch/arm64/kernel/perf_event.c
> >> @@ -864,6 +864,12 @@ 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);
> >
> > Is has_branch_stack() guaranteed to be false on arm32? If not, this will
> > be a NULL function ptr.
>
> armpmu_event_init() blocks a perf event from being created with branch
> stack sampling request without CONFIG_ARM_BRBE_PMU option being enabled
> first, which has dependency on CONFIG_ARM64. So has_branch_stack() is
> guaranteed to be false on arm32.

Then the stub functions in patch 3 are also not needed. The fact that
you create dummy functions makes it look like you can't have NULL
function ptrs, but you don't. This is what I mean about the structure
of the series being hard to review.

> static int armpmu_event_init(struct perf_event *event)
> {
> ....
> if (has_branch_stack(event)) {
> /*
> * BRBE support is absent. Select CONFIG_ARM_BRBE_PMU
> * in the config, before branch stack sampling events
> * can be requested.
> */
> if (!IS_ENABLED(CONFIG_ARM_BRBE_PMU)) {
> pr_warn_once("BRBE is disabled, select CONFIG_ARM_BRBE_PMU\n");
> return -EOPNOTSUPP;
> }
>
>
> config ARM_BRBE_PMU
> tristate "Enable support for Branch Record Buffer Extension (BRBE)"
> depends on ARM64 && ARM_PMU
> default y
> help
> Enable perf support for Branch Record Buffer Extension (BRBE) which
> records all branches taken in an execution path. This supports some
> branch types and privilege based filtering. It captured additional
> relevant information such as cycle count, misprediction and branch
> type, branch privilege level etc.
>
> >
> > To add to my other comments, this patch is where I would add
> > brbe_read(), etc. to arm_pmu.
>
> Because all new arm_pmu helpers get added and get used in the perf driver
> in the same patch, although the actual helper implementation would still
> come by bit later via the driver. This also uses updates to pmu_hw_events
> struct as well, then that patch needs to be folded here as well.
>
> There is no problem as such, kind of bit subjective. I just feel inclined
> to keep the independent infrastructure changes separate making it easy to
> review while also creating a flow.

Everything about kernel development is subjective until it's a
requirement by the maintainer. I'm not here, so it's just advice.

Rob

\
 
 \ /
  Last update: 2022-01-27 15:32    [W:0.214 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site