lkml.org 
[lkml]   [2023]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Bug report] A variant deadlock issue of CPU hot-unplug operation vs. the CFS bandwidth timer


On 2023/11/7 22:57, Thomas Gleixner wrote:
>
> Bah.
>
> So we can actually migrate the hrtimers away from the outgoing CPU in
> the dying callbacks. That's safe as nothing can queue an hrtimer remote
> on the outgoing CPU because all other CPUs are spinwaiting with
> interrupts disabled in stomp_machine() until the CPU marked itself
> offline.
>
> Survived a quick test, but needs some scrunity and probably a sanity
> check in the post dead stage.
>
> Thanks,
>
> tglx
> ---
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -195,6 +195,7 @@ enum cpuhp_state {
> CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
> CPUHP_AP_ARM64_ISNDEP_STARTING,
> CPUHP_AP_SMPCFD_DYING,
> + CPUHP_AP_HRTIMERS_DYING,
> CPUHP_AP_X86_TBOOT_DYING,
> CPUHP_AP_ARM_CACHE_B15_RAC_DYING,
> CPUHP_AP_ONLINE,
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -531,9 +531,9 @@ extern void sysrq_timer_list_show(void);
>
> int hrtimers_prepare_cpu(unsigned int cpu);
> #ifdef CONFIG_HOTPLUG_CPU
> -int hrtimers_dead_cpu(unsigned int cpu);
> +int hrtimers_cpu_dying(unsigned int cpu);
> #else
> -#define hrtimers_dead_cpu NULL
> +#define hrtimers_cpu_dying NULL
> #endif
>
> #endif
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -2116,7 +2116,7 @@ static struct cpuhp_step cpuhp_hp_states
> [CPUHP_HRTIMERS_PREPARE] = {
> .name = "hrtimers:prepare",
> .startup.single = hrtimers_prepare_cpu,
> - .teardown.single = hrtimers_dead_cpu,
> + .teardown.single = NULL,
> },
> [CPUHP_SMPCFD_PREPARE] = {
> .name = "smpcfd:prepare",
> @@ -2208,6 +2208,12 @@ static struct cpuhp_step cpuhp_hp_states
> .startup.single = NULL,
> .teardown.single = smpcfd_dying_cpu,
> },
> + [CPUHP_AP_HRTIMERS_DYING] = {
> + .name = "hrtimers:dying",
> + .startup.single = NULL,
> + .teardown.single = hrtimers_cpu_dying,
> + },
> +
> /* Entry state on starting. Interrupts enabled from here on. Transient
> * state for synchronsization */
> [CPUHP_AP_ONLINE] = {
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -2219,29 +2219,22 @@ static void migrate_hrtimer_list(struct
> }
> }
>
> -int hrtimers_dead_cpu(unsigned int scpu)
> +int hrtimers_cpu_dying(unsigned int dying_cpu)
> {
> struct hrtimer_cpu_base *old_base, *new_base;
> - int i;
> + int i, ncpu = cpumask_first(cpu_active_mask);
>
> - BUG_ON(cpu_online(scpu));
> - tick_cancel_sched_timer(scpu);
> + tick_cancel_sched_timer(dying_cpu);
> +
> + old_base = this_cpu_ptr(&hrtimer_bases);
> + new_base = &per_cpu(hrtimer_bases, ncpu);
>
> - /*
> - * this BH disable ensures that raise_softirq_irqoff() does
> - * not wakeup ksoftirqd (and acquire the pi-lock) while
> - * holding the cpu_base lock
> - */
> - local_bh_disable();
> - local_irq_disable();
> - old_base = &per_cpu(hrtimer_bases, scpu);
> - new_base = this_cpu_ptr(&hrtimer_bases);
> /*
> * The caller is globally serialized and nobody else
> * takes two locks at once, deadlock is not possible.
> */
> - raw_spin_lock(&new_base->lock);
> - raw_spin_lock_nested(&old_base->lock, SINGLE_DEPTH_NESTING);
> + raw_spin_lock(&old_base->lock);
> + raw_spin_lock_nested(&new_base->lock, SINGLE_DEPTH_NESTING);
>
> for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
> migrate_hrtimer_list(&old_base->clock_base[i],
> @@ -2252,15 +2245,13 @@ int hrtimers_dead_cpu(unsigned int scpu)
> * The migration might have changed the first expiring softirq
> * timer on this CPU. Update it.
> */
> - hrtimer_update_softirq_timer(new_base, false);
> + __hrtimer_get_next_event(new_base, HRTIMER_ACTIVE_SOFT);
> + /* Tell the other CPU to retrigger the next event */
> + smp_call_function_single(ncpu, retrigger_next_event, NULL, 0);
>
> - raw_spin_unlock(&old_base->lock);
> raw_spin_unlock(&new_base->lock);
> + raw_spin_unlock(&old_base->lock);
>
> - /* Check, if we got expired work to do */
> - __hrtimer_peek_ahead_timers();
> - local_irq_enable();
> - local_bh_enable();
> return 0;
> }
>

Thanks for the patch. Tested in v6.6 and this patch works.

Tested-by: Liu Tie <liutie4@huawei.com>

Best regards,
Tie

\
 
 \ /
  Last update: 2023-11-20 13:54    [W:0.057 / U:0.556 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site