Messages in this thread | | | Date | Tue, 29 Nov 2022 11:36:07 +0530 | Subject | Re: [PATCH V5 2/7] arm64/perf: Update struct arm_pmu for BRBE | From | Anshuman Khandual <> |
| |
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); }
| |