Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 18 Nov 2022 17:47:05 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH V5 2/7] arm64/perf: Update struct arm_pmu for BRBE |
| |
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.
| ![\](/images/icornerr.gif) |