Re: [RFC V1 06/11] arm64/perf: Drive BRBE from perf event states

[Rob Herring <robh@kernel.org>]

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