Re: [PATCH v2] arm64: perf: Fix access percpu variables in preemptible context

[Sumit Garg]

On Mon, 21 Dec 2020 at 21:53, Lecopzer Chen <lecopzer.chen@xxxxxxxxxxxx> wrote:
>
> commit 367c820ef08082 ("arm64: Enable perf events based hard lockup detector")
> reinitilizes lockup detector after arm64 PMU is initialized and open
> a window for accessing smp_processor_id() in preemptible context.
> Since hardlockup_detector_perf_init() always called in init stage
> with a single cpu, but we initialize lockup detector after the init task
> is migratable.
>
> Fix this by utilizing lockup detector reconfiguration which calls
> softlockup_start_all() on each cpu and calls watatchdog_nmi_enable() later.
> Because softlockup_start_all() use IPI call function to make sure
> watatchdog_nmi_enable() will bind on each cpu and fix this issue.

IMO, this just creates unnecessary dependency for hardlockup detector
init via softlockup detector (see the alternative definition of
lockup_detector_reconfigure()).

>
>     BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1

How about just the below fix in order to make CONFIG_DEBUG_PREEMPT happy?

diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
index 247bf0b1582c..db06ee28f48e 100644
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -165,7 +165,7 @@ static void watchdog_overflow_callback(struct
perf_event *event,

 static int hardlockup_detector_event_create(void)
 {
-       unsigned int cpu = smp_processor_id();
+       unsigned int cpu = raw_smp_processor_id();
        struct perf_event_attr *wd_attr;
        struct perf_event *evt;

-Sumit

>     caller is debug_smp_processor_id+0x20/0x2c
>     CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.10.0+ #276
>     Hardware name: linux,dummy-virt (DT)
>     Call trace:
>       dump_backtrace+0x0/0x3c0
>       show_stack+0x20/0x6c
>       dump_stack+0x2f0/0x42c
>       check_preemption_disabled+0x1cc/0x1dc
>       debug_smp_processor_id+0x20/0x2c
>       hardlockup_detector_event_create+0x34/0x18c
>       hardlockup_detector_perf_init+0x2c/0x134
>       watchdog_nmi_probe+0x18/0x24
>       lockup_detector_init+0x44/0xa8
>       armv8_pmu_driver_init+0x54/0x78
>       do_one_initcall+0x184/0x43c
>       kernel_init_freeable+0x368/0x380
>       kernel_init+0x1c/0x1cc
>       ret_from_fork+0x10/0x30
>
>
> Fixes: 367c820ef08082 ("arm64: Enable perf events based hard lockup detector")
> Signed-off-by: Lecopzer Chen <lecopzer.chen@xxxxxxxxxxxx>
> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> Cc: Sumit Garg <sumit.garg@xxxxxxxxxx>
> ---
>
> Changelog v1 -> v2:
> * https://lore.kernel.org/lkml/20201217130617.32202-1-lecopzer.chen@xxxxxxxxxxxx/
> * Move solution from kernel/watchdog_hld.c to arm64 perf_event
>     * avoid preemptive kmalloc in preempt_disable().
>
>
>
>  arch/arm64/kernel/perf_event.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 38bb07eff872..c03e21210bbb 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -1345,4 +1345,20 @@ u64 hw_nmi_get_sample_period(int watchdog_thresh)
>
>         return (u64)max_cpu_freq * watchdog_thresh;
>  }
> +
> +/*
> + * hardlockup_detector_perf_init() always call in init stage with a single
> + * cpu. In arm64 case, we re-initialize lockup detector after pmu driver
> + * initialized. Lockup detector initial function use lots of percpu variables
> + * and this makes CONFIG_DEBUG_PREEMPT unhappy because we are now in
> + * preemptive context.
> + * Return 0 if the nmi is ready and register nmi hardlockup detector by
> + * lockup detector reconfiguration.
> + */
> +int __init watchdog_nmi_probe(void)
> +{
> +       if (arm_pmu_irq_is_nmi())
> +               return 0;
> +       return -ENODEV;
> +}
>  #endif
> --
> 2.25.1
>