RE: [PATCH v2] perf: arm_cspmu: Separate Arm and vendor module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Suzuki,

> -----Original Message-----
> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Sent: Wednesday, April 19, 2023 4:32 PM
> To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>; catalin.marinas@xxxxxxx;
> will@xxxxxxxxxx; mark.rutland@xxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> tegra@xxxxxxxxxxxxxxx; Thierry Reding <treding@xxxxxxxxxx>; Jonathan
> Hunter <jonathanh@xxxxxxxxxx>; Vikram Sethi <vsethi@xxxxxxxxxx>; Richard
> Wiley <rwiley@xxxxxxxxxx>; Eric Funsten <efunsten@xxxxxxxxxx>
> Subject: Re: [PATCH v2] perf: arm_cspmu: Separate Arm and vendor module
> 
> External email: Use caution opening links or attachments
> 
> 
> On 18/04/2023 20:33, Besar Wicaksono wrote:
> > Hi Suzuki,
> >
> >> -----Original Message-----
> >> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> >> Sent: Tuesday, April 18, 2023 6:07 AM
> >> To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>;
> catalin.marinas@xxxxxxx;
> >> will@xxxxxxxxxx; mark.rutland@xxxxxxx
> >> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-
> >> tegra@xxxxxxxxxxxxxxx; Thierry Reding <treding@xxxxxxxxxx>; Jonathan
> >> Hunter <jonathanh@xxxxxxxxxx>; Vikram Sethi <vsethi@xxxxxxxxxx>;
> Richard
> >> Wiley <rwiley@xxxxxxxxxx>; Eric Funsten <efunsten@xxxxxxxxxx>
> >> Subject: Re: [PATCH v2] perf: arm_cspmu: Separate Arm and vendor
> module
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> On 18/04/2023 07:20, Besar Wicaksono wrote:
> >>> Arm Coresight PMU driver consists of main standard code and vendor
> >>> backend code. Both are currently built as a single module.
> >>> This patch adds vendor registration API to separate the two to
> >>> keep things modular. Vendor module shall register to the main
> >>> module on loading and trigger device reprobe.
> >>>
> >>> Signed-off-by: Besar Wicaksono <bwicaksono@xxxxxxxxxx>
> >>> ---
> >>>
> >>> Changes from v1:
> >>>    * Added separate Kconfig entry for nvidia backend
> >>>    * Added lock to protect accesses to the lists
> >>>    * Added support for matching subset devices from a vendor
> >>>    * Added state tracking to avoid reprobe when a device is in use
> >>> v1: ttps://lore.kernel.org/linux-arm-kernel/20230403163905.20354-1-
> >> bwicaksono@xxxxxxxxxx/T/#u
> >>>
> >>> ---
> >>>    drivers/perf/arm_cspmu/Kconfig        |   9 +-
> >>>    drivers/perf/arm_cspmu/Makefile       |   6 +-
> >>>    drivers/perf/arm_cspmu/arm_cspmu.c    | 280
> >> +++++++++++++++++++++++---
> >>>    drivers/perf/arm_cspmu/arm_cspmu.h    |  32 ++-
> >>>    drivers/perf/arm_cspmu/nvidia_cspmu.c |  39 +++-
> >>>    drivers/perf/arm_cspmu/nvidia_cspmu.h |  17 --
> >>>    6 files changed, 325 insertions(+), 58 deletions(-)
> >>>    delete mode 100644 drivers/perf/arm_cspmu/nvidia_cspmu.h
> >>>
> >>> diff --git a/drivers/perf/arm_cspmu/Kconfig
> >> b/drivers/perf/arm_cspmu/Kconfig
> >>> index 0b316fe69a45..8ce7b45a0075 100644
> >>> --- a/drivers/perf/arm_cspmu/Kconfig
> >>> +++ b/drivers/perf/arm_cspmu/Kconfig
> >>> @@ -1,6 +1,6 @@
> >>>    # SPDX-License-Identifier: GPL-2.0
> >>>    #
> >>> -# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights
> >> reserved.
> >>> +# Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All
> rights
> >> reserved.
> >>>
> >>>    config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
> >>>        tristate "ARM Coresight Architecture PMU"
> >>> @@ -11,3 +11,10 @@ config
> ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
> >>>          based on ARM CoreSight PMU architecture. Note that this PMU
> >>>          architecture does not have relationship with the ARM CoreSight
> >>>          Self-Hosted Tracing.
> >>> +
> >>> +config NVIDIA_CORESIGHT_PMU_ARCH_SYSTEM_PMU
> >>> +     tristate "NVIDIA Coresight Architecture PMU"
> >>> +     depends on ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
> >>> +     help
> >>> +       Provides NVIDIA specific attributes for performance monitoring unit
> >>> +       (PMU) devices based on ARM CoreSight PMU architecture.
> >>> diff --git a/drivers/perf/arm_cspmu/Makefile
> >> b/drivers/perf/arm_cspmu/Makefile
> >>> index fedb17df982d..f8ae22411d59 100644
> >>> --- a/drivers/perf/arm_cspmu/Makefile
> >>> +++ b/drivers/perf/arm_cspmu/Makefile
> >>> @@ -1,6 +1,6 @@
> >>> -# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights
> >> reserved.
> >>> +# Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All
> rights
> >> reserved.
> >>>    #
> >>>    # SPDX-License-Identifier: GPL-2.0
> >>>
> >>> -obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) +=
> >> arm_cspmu_module.o
> >>> -arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o
> >>> +obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) +=
> >> arm_cspmu.o
> >>> +obj-$(CONFIG_NVIDIA_CORESIGHT_PMU_ARCH_SYSTEM_PMU) +=
> >> nvidia_cspmu.o
> >>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> >> b/drivers/perf/arm_cspmu/arm_cspmu.c
> >>> index e31302ab7e37..c55ea2b74454 100644
> >>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> >>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> >>> @@ -16,7 +16,7 @@
> >>>     * The user should refer to the vendor technical documentation to get
> >> details
> >>>     * about the supported events.
> >>>     *
> >>> - * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights
> >> reserved.
> >>> + * Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All
> rights
> >> reserved.
> >>>     *
> >>>     */
> >>>
> >>> @@ -25,13 +25,14 @@
> >>>    #include <linux/ctype.h>
> >>>    #include <linux/interrupt.h>
> >>>    #include <linux/io-64-nonatomic-lo-hi.h>
> >>> +#include <linux/list.h>
> >>>    #include <linux/module.h>
> >>> +#include <linux/mutex.h>
> >>>    #include <linux/perf_event.h>
> >>>    #include <linux/platform_device.h>
> >>>    #include <acpi/processor.h>
> >>>
> >>>    #include "arm_cspmu.h"
> >>> -#include "nvidia_cspmu.h"
> >>>
> >>>    #define PMUNAME "arm_cspmu"
> >>>    #define DRVNAME "arm-cs-arch-pmu"
> >>> @@ -117,11 +118,52 @@
> >>>     */
> >>>    #define HILOHI_MAX_POLL     1000
> >>>
> >>> -/* JEDEC-assigned JEP106 identification code */
> >>> -#define ARM_CSPMU_IMPL_ID_NVIDIA             0x36B
> >>> -
> >>>    static unsigned long arm_cspmu_cpuhp_state;
> >>>
> >>> +/* List of Coresight PMU instances in the system. */
> >>> +static LIST_HEAD(arm_cspmus);
> >>> +
> >>> +/* List of registered vendor backends. */
> >>> +static LIST_HEAD(arm_cspmu_impls);
> >>> +
> >>> +static DEFINE_MUTEX(arm_cspmu_lock);
> >>> +
> >>> +/*
> >>> + * State of the generic driver.
> >>> + * 0 => registering backend.
> >>> + * 1 => ready to use.
> >>> + * 2 or more => in use.
> >>> + */
> >>> +#define ARM_CSPMU_STATE_REG  0
> >>> +#define ARM_CSPMU_STATE_READY        1
> >>> +static atomic_t arm_cspmu_state;
> >>> +
> >>> +static void arm_cspmu_state_ready(void)
> >>> +{
> >>> +     atomic_set(&arm_cspmu_state, ARM_CSPMU_STATE_READY);
> >>> +}
> >>> +
> >>> +static bool try_arm_cspmu_state_reg(void)
> >>> +{
> >>> +     const int old = ARM_CSPMU_STATE_READY;
> >>> +     const int new = ARM_CSPMU_STATE_REG;
> >>> +
> >>> +     return atomic_cmpxchg(&arm_cspmu_state, old, new) == old;
> >>> +}
> >>> +
> >>> +static bool try_arm_cspmu_state_get(void)
> >>> +{
> >>> +     return atomic_inc_not_zero(&arm_cspmu_state);
> >>> +}
> >>> +
> >>> +static void arm_cspmu_state_put(void)
> >>> +{
> >>> +     int ret;
> >>> +
> >>> +     ret = atomic_dec_if_positive(&arm_cspmu_state);
> >>> +     WARN_ON(ret < 0);
> >>> +}
> >>> +
> >>
> >> As long as the vendor module is set for the PMU instance, it won't be
> >> unloaded as long as there are any perf events and thus the specific
> >> driver cannot be unloaded. So, you don't need explicit refcount
> >> maintenance for each pmu callbacks.
> >>
> >
> > The arm_cspmu_state mainly protects during new backend registration
> when
> > the device is attached to standard implementation. All devices are attached
> to
> > standard implementation initially when arm_cspmu module is loaded, since
> there
> > is no backend yet. On backend registration, the standard impl is replaced by
> > backend impl. However, the module unloading mechanism doesn't provide
> > protection because standard impl is owned by arm_cspmu module, which
> > is not unloaded during registration.
> >
> > The refcount may not be required if the devices are not attached to standard
> > Implementation by default, and the unreg doesn't fallback to it. But that
> makes
> > the devices usable only when there is a vendor backend available.
> 
> Ok, thanks for the explanation. But I still think we :
> 
>   - Don't need a single global refcount for all the PMUs. Instead this
>     could be per PMU instance (arm_cspmu), only when it is backed by

Ok, we can add refcount per PMU.

>     "generic" backend, others get module refcount. If the refcount of
>     "generic" PMU is positive, during the registration of a matching
>     backend driver, we could simply mark that as pending reprobe.
> 
>   - And only do the refcount for the following call backs:
> 
>    pmu:: event_init -> hold the refcount
>    pmu:: destroy -> drop the refcount and trigger a reprobe if one was
>          pending (see above)

Is it safe to reprobe on destroy ? The reprobe will free and reallocate
the memory managed by the device.

Thanks,
Besar




[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux