| Message ID | 20221107062514.2851047-3-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/perf: Enable branch stack sampling | expand |
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. Suzuki
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/c3804290-bdb1-d1eb-3526-9b0ce4c8e8b1@arm.com/ -------------------------------------------------------------------------- > > 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. - Anshuman
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/c3804290-bdb1-d1eb-3526-9b0ce4c8e8b1@arm.com/ > > -------------------------------------------------------------------------- > > > > 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. Thanks, Mark.
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/c3804290-bdb1-d1eb-3526-9b0ce4c8e8b1@arm.com/ >> >> -------------------------------------------------------------------------- >>> >>> 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); }
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index 7b0643fe2f13..c97377e28288 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -1025,6 +1025,35 @@ static int armv8pmu_filter_match(struct perf_event *event) return evtype != ARMV8_PMUV3_PERFCTR_CHAIN; } +static void armv8pmu_brbe_filter(struct pmu_hw_events *hw_event, struct perf_event *event) +{ +} + +static void armv8pmu_brbe_enable(struct pmu_hw_events *hw_event) +{ +} + +static void armv8pmu_brbe_disable(struct pmu_hw_events *hw_event) +{ +} + +static void armv8pmu_brbe_read(struct pmu_hw_events *hw_event, struct perf_event *event) +{ +} + +static void armv8pmu_brbe_probe(struct pmu_hw_events *hw_event) +{ +} + +static void armv8pmu_brbe_reset(struct pmu_hw_events *hw_event) +{ +} + +static bool armv8pmu_brbe_supported(struct perf_event *event) +{ + return false; +} + static void armv8pmu_reset(void *info) { struct arm_pmu *cpu_pmu = (struct arm_pmu *)info; @@ -1257,6 +1286,13 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name, cpu_pmu->pmu.event_idx = armv8pmu_user_event_idx; + cpu_pmu->brbe_filter = armv8pmu_brbe_filter; + cpu_pmu->brbe_enable = armv8pmu_brbe_enable; + cpu_pmu->brbe_disable = armv8pmu_brbe_disable; + cpu_pmu->brbe_read = armv8pmu_brbe_read; + cpu_pmu->brbe_probe = armv8pmu_brbe_probe; + cpu_pmu->brbe_reset = armv8pmu_brbe_reset; + cpu_pmu->brbe_supported = armv8pmu_brbe_supported; cpu_pmu->name = name; cpu_pmu->map_event = map_event; cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_EVENTS] = events ? diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h index 0356cb6a215d..67a6d59786f2 100644 --- a/include/linux/perf/arm_pmu.h +++ b/include/linux/perf/arm_pmu.h @@ -101,6 +101,27 @@ struct arm_pmu { void (*reset)(void *); int (*map_event)(struct perf_event *event); int (*filter_match)(struct perf_event *event); + + /* Convert perf event filters into BRBE HW filters */ + void (*brbe_filter)(struct pmu_hw_events *hw_events, struct perf_event *event); + + /* Probe BRBE HW and capture its attributes */ + void (*brbe_probe)(struct pmu_hw_events *hw_events); + + /* Enable BRBE HW with a given config */ + void (*brbe_enable)(struct pmu_hw_events *hw_events); + + /* Disable BRBE HW */ + void (*brbe_disable)(struct pmu_hw_events *hw_events); + + /* Process BRBE buffer for captured branch records */ + void (*brbe_read)(struct pmu_hw_events *hw_events, struct perf_event *event); + + /* Reset BRBE buffer */ + void (*brbe_reset)(struct pmu_hw_events *hw_events); + + /* Check whether BRBE is supported */ + bool (*brbe_supported)(struct perf_event *event); int num_events; bool secure_access; /* 32-bit ARM only */ #define ARMV8_PMUV3_MAX_COMMON_EVENTS 0x40
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. Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-perf-users@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/arm64/kernel/perf_event.c | 36 ++++++++++++++++++++++++++++++++++ include/linux/perf/arm_pmu.h | 21 ++++++++++++++++++++ 2 files changed, 57 insertions(+)