[WARNING] tick_handle_oneshot_broadcast() on !online CPU

[WARNING] tick_handle_oneshot_broadcast() on !online CPU

[Valentin Schneider]

Hi,

I hit this while running some hotplug stress tests on my HiKey960 with:

  babf09c3837f ("Merge tag 'tag-chrome-platform-fixes-for-v5.1-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux")

[   13.181753] WARNING: CPU: 4 PID: 29 at kernel/time/tick-broadcast.c:647 tick_handle_oneshot_broadcast+0x1b4/0x1d0
[   13.192342] Modules linked in:
[   13.195428] CPU: 4 PID: 29 Comm: migration/4 Tainted: G S                5.1.0-rc1-00015-gbabf09c3837f #23
[   13.205140] Hardware name: HiKey960 (DT)
[   13.209089] pstate: 00000085 (nzcv daIf -PAN -UAO)
[   13.213918] pc : tick_handle_oneshot_broadcast+0x1b4/0x1d0
[   13.219440] lr : tick_handle_oneshot_broadcast+0x148/0x1d0
[   13.224958] sp : ffff000010023b50
[   13.228294] x29: ffff000010023b50 x28: ffff8000ba12a000
[   13.233642] x27: ffff000012352c70 x26: ffff000011706e80
[   13.238989] x25: ffff0000114fa000 x24: 00000002fdabdc71
[   13.244335] x23: ffff000012352ce0 x22: ffff000011504080
[   13.249681] x21: ffff000012352cb0 x20: 0000000000000000
[   13.255027] x19: 7fffffffffffffff x18: 0000000000000000
[   13.260373] x17: 0000000000000000 x16: 0000000000000000
[   13.265719] x15: 000000000000043b x14: 0000000000000000
[   13.271064] x13: 0000000000000000 x12: ffff00001176e9f8
[   13.276409] x11: 00000000000000f3 x10: 0000000000000040
[   13.281755] x9 : 0000000000000000 x8 : ffff000011503808
[   13.287101] x7 : ffff000011506000 x6 : 0000000000000004
[   13.292446] x5 : 0000000000000020 x4 : 0000000000000010
[   13.297792] x3 : 0000000000000000 x2 : 0000000000000100
[   13.303138] x1 : ffff000011504080 x0 : 0000000000000000
[   13.308485] Call trace:
[   13.310951]  tick_handle_oneshot_broadcast+0x1b4/0x1d0
[   13.316130]  sp804_timer_interrupt+0x30/0x40
[   13.320432]  __handle_irq_event_percpu+0xb8/0x460
[   13.325165]  handle_irq_event_percpu+0x34/0x88
[   13.329636]  handle_irq_event+0x48/0x78
[   13.333501]  handle_fasteoi_irq+0xb4/0x188
[   13.337626]  generic_handle_irq+0x24/0x38
[   13.341661]  __handle_domain_irq+0x5c/0xb0
[   13.345789]  gic_handle_irq+0x58/0xa8
[   13.349475]  el1_irq+0xbc/0x180
[   13.352636]  __do_softirq+0xcc/0x574
[   13.356239]  irq_exit+0x13c/0x148
[   13.359577]  __handle_domain_irq+0x60/0xb0
[   13.363698]  gic_handle_irq+0x58/0xa8
[   13.367383]  el1_irq+0xbc/0x180
[   13.370549]  multi_cpu_stop+0xf8/0x170
[   13.374323]  cpu_stopper_thread+0xac/0x120
[   13.378451]  smpboot_thread_fn+0x16c/0x270
[   13.382575]  kthread+0x11c/0x120
[   13.385826]  ret_from_fork+0x10/0x18
[   13.389424] irq event stamp: 533
[   13.392676] hardirqs last  enabled at (532): [<ffff000010081c74>] __do_softirq+0xc4/0x574
[   13.400907] hardirqs last disabled at (533): [<ffff000010083c7c>] el1_irq+0x7c/0x180
[   13.408702] softirqs last  enabled at (526): [<ffff000010082024>] __do_softirq+0x474/0x574
[   13.417021] softirqs last disabled at (531): [<ffff0000100f276c>] irq_exit+0x13c/0x148
[   13.424987] ---[ end trace c9d39afcc503fe70 ]---
[   13.429773] CPU2: shutdown
[   13.437515] psci: CPU2 killed.

I stared at the code and did a bit of tracing, the sequence seems to be:

---
echo 0 > /sys/devices/system/cpu/cpu2/online

takedown_cpu()
  take_cpu_down()
     __cpu_disable() (clears CPU in cpu_online_mask & cpu_active_mask)

  [...] <warn happens here>

  tick_cleanup_dead_cpu()
    tick_shutdown_broadcast_oneshot() (removes cpu from the tick_broadcast_* masks)
---

In that case we always have

  tick_broadcast_force_mask=[CPU2]
  cpu_online_mask=[CPU4]

So tick_handle_oneshot_broadcast::tmpmask becomes [CPU2] and we hit the warn.



I was thinking of guarding the setting of tmpmask with cpu_online(cpu), but
AFAICT nothing saves us from __cpu_disable() happening *after* those checks
(and even potentially after the WARN_ON_ONCE). Do we want some extra lock
in here, or is that a benign issue?



Appended is my reproducer - it comes from a longer script that serves as a
hotplug torture test, but these few lines are enough to trigger the above.

-----8<-----

#!/bin/bash

echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 0.085
echo 0 > /sys/devices/system/cpu/cpu5/online
sleep 0.08
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.018
echo 0 > /sys/devices/system/cpu/cpu3/online
sleep 0.05
echo 0 > /sys/devices/system/cpu/cpu0/online
sleep 0.049
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.064
echo 0 > /sys/devices/system/cpu/cpu6/online
sleep 0.037
echo 0 > /sys/devices/system/cpu/cpu7/online
sleep 0.033
echo 0 > /sys/devices/system/cpu/cpu2/online
sleep 0.025
echo 1 > /sys/devices/system/cpu/cpu2/online
sleep 0.018
echo 1 > /sys/devices/system/cpu/cpu7/online
sleep 0.069
echo 0 > /sys/devices/system/cpu/cpu7/online
sleep 0.036
echo 0 > /sys/devices/system/cpu/cpu4/online
sleep 0.06
echo 1 > /sys/devices/system/cpu/cpu3/online
sleep 0.067
echo 1 > /sys/devices/system/cpu/cpu4/online
sleep 0.055
echo 1 > /sys/devices/system/cpu/cpu1/online
sleep 0.052
echo 1 > /sys/devices/system/cpu/cpu6/online
sleep 0.035
echo 1 > /sys/devices/system/cpu/cpu7/online
sleep 0.053

Re: [WARNING] tick_handle_oneshot_broadcast() on !online CPU

[Thomas Gleixner]

On Thu, 21 Mar 2019, Valentin Schneider wrote:
> I stared at the code and did a bit of tracing, the sequence seems to be:
> 
> ---
> echo 0 > /sys/devices/system/cpu/cpu2/online
> 
> takedown_cpu()
>   take_cpu_down()
>      __cpu_disable() (clears CPU in cpu_online_mask & cpu_active_mask)

active_mask has been cleared long ago.
 
>   [...] <warn happens here>
> 
>   tick_cleanup_dead_cpu()
>     tick_shutdown_broadcast_oneshot() (removes cpu from the tick_broadcast_* masks)
> ---
> 
> In that case we always have
> 
>   tick_broadcast_force_mask=[CPU2]
>   cpu_online_mask=[CPU4]
> 
> So tick_handle_oneshot_broadcast::tmpmask becomes [CPU2] and we hit the warn.
>
> 
> I was thinking of guarding the setting of tmpmask with cpu_online(cpu), but
> AFAICT nothing saves us from __cpu_disable() happening *after* those checks
> (and even potentially after the WARN_ON_ONCE). Do we want some extra lock
> in here, or is that a benign issue?

Hmm. This only seems to be an issue with forced broadcast. The dynamic
broadcast stuff does not have that problem as the CPU is obviously not idle
while going down.

Patch below (completely untested) should fix it.

Thanks,

	tglx

8<---------------

--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -68,6 +68,12 @@ extern void tick_broadcast_control(enum
 static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
 #endif /* BROADCAST */
 
+#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
+extern void tick_offline_cpu(unsigned int cpu);
+#else
+static inline void tick_offline_cpu(unsigned int cpu) { }
+#endif
+
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
 #else
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -844,6 +844,8 @@ static int take_cpu_down(void *_param)
 
 	/* Give up timekeeping duties */
 	tick_handover_do_timer();
+	/* Remove CPU from timer broadcasting */
+	tick_offline_cpu(cpu);
 	/* Park the stopper thread */
 	stop_machine_park(cpu);
 	return 0;
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -611,6 +611,22 @@ void clockevents_resume(void)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
+
+# ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+/**
+ * tick_offline_cpu - Take CPU out of the broadcast mechanism
+ * @cpu:	The outgoing CPU
+ *
+ * Called on the outgoing CPU after it took itself offline.
+ */
+void tick_offline_cpu(unsigned int cpu)
+{
+	raw_spin_lock(&clockevents_lock);
+	tick_broadcast_offline(cpu);
+	raw_spin_unlock(&clockevents_lock);
+}
+# endif
+
 /**
  * tick_cleanup_dead_cpu - Cleanup the tick and clockevents of a dead cpu
  */
@@ -621,8 +637,6 @@ void tick_cleanup_dead_cpu(int cpu)
 
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
 
-	tick_shutdown_broadcast_oneshot(cpu);
-	tick_shutdown_broadcast(cpu);
 	tick_shutdown(cpu);
 	/*
 	 * Unregister the clock event devices which were
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -36,10 +36,12 @@ static __cacheline_aligned_in_smp DEFINE
 static void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
 static void tick_broadcast_clear_oneshot(int cpu);
 static void tick_resume_broadcast_oneshot(struct clock_event_device *bc);
+static void tick_broadcast_oneshot_offline(unsigned int cpu);
 #else
 static inline void tick_broadcast_setup_oneshot(struct clock_event_device *bc) { BUG(); }
 static inline void tick_broadcast_clear_oneshot(int cpu) { }
 static inline void tick_resume_broadcast_oneshot(struct clock_event_device *bc) { }
+static inline void tick_broadcast_oneshot_offline(unsigned int cpu) { }
 #endif
 
 /*
@@ -444,8 +446,6 @@ void tick_shutdown_broadcast(unsigned in
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 
 	bc = tick_broadcast_device.evtdev;
-	cpumask_clear_cpu(cpu, tick_broadcast_mask);
-	cpumask_clear_cpu(cpu, tick_broadcast_on);
 
 	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
 		if (bc && cpumask_empty(tick_broadcast_mask))
@@ -454,6 +454,16 @@ void tick_shutdown_broadcast(unsigned in
 
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
+
+void tick_broadcast_offline(unsigned int cpu)
+{
+	raw_spin_lock(&tick_broadcast_lock);
+	cpumask_clear_cpu(cpu, tick_broadcast_mask);
+	cpumask_clear_cpu(cpu, tick_broadcast_on);
+	tick_broadcast_oneshot_offline(cpu);
+	raw_spin_unlock(&tick_broadcast_lock);
+}
+
 #endif
 
 void tick_suspend_broadcast(void)
@@ -950,14 +960,10 @@ void hotplug_cpu__broadcast_tick_pull(in
 }
 
 /*
- * Remove a dead CPU from broadcasting
+ * Remove a dying CPU from broadcasting
  */
-void tick_shutdown_broadcast_oneshot(unsigned int cpu)
+static void tick_broadcast_oneshot_offline(unsigned int cpu)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
-
 	/*
 	 * Clear the broadcast masks for the dead cpu, but do not stop
 	 * the broadcast device!
@@ -965,8 +971,6 @@ void tick_shutdown_broadcast_oneshot(uns
 	cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
 	cpumask_clear_cpu(cpu, tick_broadcast_pending_mask);
 	cpumask_clear_cpu(cpu, tick_broadcast_force_mask);
-
-	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 #endif
 
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -64,7 +64,6 @@ extern ssize_t sysfs_get_uname(const cha
 extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu);
 extern void tick_install_broadcast_device(struct clock_event_device *dev);
 extern int tick_is_broadcast_device(struct clock_event_device *dev);
-extern void tick_shutdown_broadcast(unsigned int cpu);
 extern void tick_suspend_broadcast(void);
 extern void tick_resume_broadcast(void);
 extern bool tick_resume_check_broadcast(void);
@@ -78,7 +77,6 @@ static inline void tick_install_broadcas
 static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; }
 static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; }
 static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }
-static inline void tick_shutdown_broadcast(unsigned int cpu) { }
 static inline void tick_suspend_broadcast(void) { }
 static inline void tick_resume_broadcast(void) { }
 static inline bool tick_resume_check_broadcast(void) { return false; }
@@ -128,19 +126,23 @@ static inline int tick_check_oneshot_cha
 /* Functions related to oneshot broadcasting */
 #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
 extern void tick_broadcast_switch_to_oneshot(void);
-extern void tick_shutdown_broadcast_oneshot(unsigned int cpu);
 extern int tick_broadcast_oneshot_active(void);
 extern void tick_check_oneshot_broadcast_this_cpu(void);
 bool tick_broadcast_oneshot_available(void);
 extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
 #else /* !(BROADCAST && ONESHOT): */
 static inline void tick_broadcast_switch_to_oneshot(void) { }
-static inline void tick_shutdown_broadcast_oneshot(unsigned int cpu) { }
 static inline int tick_broadcast_oneshot_active(void) { return 0; }
 static inline void tick_check_oneshot_broadcast_this_cpu(void) { }
 static inline bool tick_broadcast_oneshot_available(void) { return tick_oneshot_possible(); }
 #endif /* !(BROADCAST && ONESHOT) */
 
+#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
+extern void tick_broadcast_offline(unsigned int cpu);
+#else
+static inline void tick_broadcast_offline(unsigned int cpu) { }
+#endif
+
 /* NO_HZ_FULL internal */
 #ifdef CONFIG_NO_HZ_FULL
 extern void tick_nohz_init(void);

Re: [WARNING] tick_handle_oneshot_broadcast() on !online CPU

[Valentin Schneider]

On 21/03/2019 15:39, Thomas Gleixner wrote:
> On Thu, 21 Mar 2019, Valentin Schneider wrote:
>> I stared at the code and did a bit of tracing, the sequence seems to be:
>>
>> ---
>> echo 0 > /sys/devices/system/cpu/cpu2/online
>>
>> takedown_cpu()
>>   take_cpu_down()
>>      __cpu_disable() (clears CPU in cpu_online_mask & cpu_active_mask)
> 
> active_mask has been cleared long ago.

Right, dunno where I picked that up...

[...]
> Hmm. This only seems to be an issue with forced broadcast. The dynamic
> broadcast stuff does not have that problem as the CPU is obviously not idle
> while going down.
> 
> Patch below (completely untested) should fix it.
> 
> Thanks,
> 

No more warns after a few hundred iterations, and the tick_offline_cpu()
in take_cpu_down() makes sense to me.

Tested-by: Valentin Schneider <valentin.schneider@arm.com>

Thanks!
Valentin

> 	tglx
> 
> 8<---------------
> 
> --- a/include/linux/tick.h
> +++ b/include/linux/tick.h
> @@ -68,6 +68,12 @@ extern void tick_broadcast_control(enum
>  static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
>  #endif /* BROADCAST */
>  
> +#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
> +extern void tick_offline_cpu(unsigned int cpu);
> +#else
> +static inline void tick_offline_cpu(unsigned int cpu) { }
> +#endif
> +
>  #ifdef CONFIG_GENERIC_CLOCKEVENTS
>  extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
>  #else
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -844,6 +844,8 @@ static int take_cpu_down(void *_param)
>  
>  	/* Give up timekeeping duties */
>  	tick_handover_do_timer();
> +	/* Remove CPU from timer broadcasting */
> +	tick_offline_cpu(cpu);
>  	/* Park the stopper thread */
>  	stop_machine_park(cpu);
>  	return 0;
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -611,6 +611,22 @@ void clockevents_resume(void)
>  }
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> +
> +# ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> +/**
> + * tick_offline_cpu - Take CPU out of the broadcast mechanism
> + * @cpu:	The outgoing CPU
> + *
> + * Called on the outgoing CPU after it took itself offline.
> + */
> +void tick_offline_cpu(unsigned int cpu)
> +{
> +	raw_spin_lock(&clockevents_lock);
> +	tick_broadcast_offline(cpu);
> +	raw_spin_unlock(&clockevents_lock);
> +}
> +# endif
> +
>  /**
>   * tick_cleanup_dead_cpu - Cleanup the tick and clockevents of a dead cpu
>   */
> @@ -621,8 +637,6 @@ void tick_cleanup_dead_cpu(int cpu)
>  
>  	raw_spin_lock_irqsave(&clockevents_lock, flags);
>  
> -	tick_shutdown_broadcast_oneshot(cpu);
> -	tick_shutdown_broadcast(cpu);
>  	tick_shutdown(cpu);
>  	/*
>  	 * Unregister the clock event devices which were
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -36,10 +36,12 @@ static __cacheline_aligned_in_smp DEFINE
>  static void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
>  static void tick_broadcast_clear_oneshot(int cpu);
>  static void tick_resume_broadcast_oneshot(struct clock_event_device *bc);
> +static void tick_broadcast_oneshot_offline(unsigned int cpu);
>  #else
>  static inline void tick_broadcast_setup_oneshot(struct clock_event_device *bc) { BUG(); }
>  static inline void tick_broadcast_clear_oneshot(int cpu) { }
>  static inline void tick_resume_broadcast_oneshot(struct clock_event_device *bc) { }
> +static inline void tick_broadcast_oneshot_offline(unsigned int cpu) { }
>  #endif
>  
>  /*
> @@ -444,8 +446,6 @@ void tick_shutdown_broadcast(unsigned in
>  	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
>  
>  	bc = tick_broadcast_device.evtdev;
> -	cpumask_clear_cpu(cpu, tick_broadcast_mask);
> -	cpumask_clear_cpu(cpu, tick_broadcast_on);
>  
>  	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
>  		if (bc && cpumask_empty(tick_broadcast_mask))
> @@ -454,6 +454,16 @@ void tick_shutdown_broadcast(unsigned in
>  
>  	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
>  }
> +
> +void tick_broadcast_offline(unsigned int cpu)
> +{
> +	raw_spin_lock(&tick_broadcast_lock);
> +	cpumask_clear_cpu(cpu, tick_broadcast_mask);
> +	cpumask_clear_cpu(cpu, tick_broadcast_on);
> +	tick_broadcast_oneshot_offline(cpu);
> +	raw_spin_unlock(&tick_broadcast_lock);
> +}
> +
>  #endif
>  
>  void tick_suspend_broadcast(void)
> @@ -950,14 +960,10 @@ void hotplug_cpu__broadcast_tick_pull(in
>  }
>  
>  /*
> - * Remove a dead CPU from broadcasting
> + * Remove a dying CPU from broadcasting
>   */
> -void tick_shutdown_broadcast_oneshot(unsigned int cpu)
> +static void tick_broadcast_oneshot_offline(unsigned int cpu)
>  {
> -	unsigned long flags;
> -
> -	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> -
>  	/*
>  	 * Clear the broadcast masks for the dead cpu, but do not stop
>  	 * the broadcast device!
> @@ -965,8 +971,6 @@ void tick_shutdown_broadcast_oneshot(uns
>  	cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
>  	cpumask_clear_cpu(cpu, tick_broadcast_pending_mask);
>  	cpumask_clear_cpu(cpu, tick_broadcast_force_mask);
> -
> -	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
>  }
>  #endif
>  
> --- a/kernel/time/tick-internal.h
> +++ b/kernel/time/tick-internal.h
> @@ -64,7 +64,6 @@ extern ssize_t sysfs_get_uname(const cha
>  extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu);
>  extern void tick_install_broadcast_device(struct clock_event_device *dev);
>  extern int tick_is_broadcast_device(struct clock_event_device *dev);
> -extern void tick_shutdown_broadcast(unsigned int cpu);
>  extern void tick_suspend_broadcast(void);
>  extern void tick_resume_broadcast(void);
>  extern bool tick_resume_check_broadcast(void);
> @@ -78,7 +77,6 @@ static inline void tick_install_broadcas
>  static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; }
>  static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; }
>  static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }
> -static inline void tick_shutdown_broadcast(unsigned int cpu) { }
>  static inline void tick_suspend_broadcast(void) { }
>  static inline void tick_resume_broadcast(void) { }
>  static inline bool tick_resume_check_broadcast(void) { return false; }
> @@ -128,19 +126,23 @@ static inline int tick_check_oneshot_cha
>  /* Functions related to oneshot broadcasting */
>  #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
>  extern void tick_broadcast_switch_to_oneshot(void);
> -extern void tick_shutdown_broadcast_oneshot(unsigned int cpu);
>  extern int tick_broadcast_oneshot_active(void);
>  extern void tick_check_oneshot_broadcast_this_cpu(void);
>  bool tick_broadcast_oneshot_available(void);
>  extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
>  #else /* !(BROADCAST && ONESHOT): */
>  static inline void tick_broadcast_switch_to_oneshot(void) { }
> -static inline void tick_shutdown_broadcast_oneshot(unsigned int cpu) { }
>  static inline int tick_broadcast_oneshot_active(void) { return 0; }
>  static inline void tick_check_oneshot_broadcast_this_cpu(void) { }
>  static inline bool tick_broadcast_oneshot_available(void) { return tick_oneshot_possible(); }
>  #endif /* !(BROADCAST && ONESHOT) */
>  
> +#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
> +extern void tick_broadcast_offline(unsigned int cpu);
> +#else
> +static inline void tick_broadcast_offline(unsigned int cpu) { }
> +#endif
> +
>  /* NO_HZ_FULL internal */
>  #ifdef CONFIG_NO_HZ_FULL
>  extern void tick_nohz_init(void);
> 

[tip:timers/core] tick: Remove outgoing CPU from broadcast masks

[tip-bot for Thomas Gleixner]

Commit-ID:  973f88ca8b159592685b6ccedb0b91db376bf20d
Gitweb:     https://git.kernel.org/tip/973f88ca8b159592685b6ccedb0b91db376bf20d
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 21 Mar 2019 16:39:20 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 23 Mar 2019 12:21:32 +0100

tick: Remove outgoing CPU from broadcast masks

Valentin reported that unplugging a CPU occasionally results in a warning
in the tick broadcast code which is triggered when an offline CPU is in the
broadcast mask.

This happens because the outgoing CPU is not removing itself from the
broadcast masks, especially not from the broadcast_force_mask. The removal
happens on the control CPU after the outgoing CPU is dead. It's a long
standing issue, but the warning is harmless.

Rework the hotplug mechanism so that the outgoing CPU removes itself from
the broadcast masks after disabling interrupts and removing itself from the
online mask.

Reported-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Valentin Schneider <valentin.schneider@arm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1903211540180.1784@nanos.tec.linutronix.de

---
 include/linux/tick.h         |  6 ++++++
 kernel/cpu.c                 |  2 ++
 kernel/time/clockevents.c    | 18 ++++++++++++++++--
 kernel/time/tick-broadcast.c | 24 ++++++++++++++----------
 kernel/time/tick-internal.h  | 10 ++++++----
 5 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 55388ab45fd4..76acb48acdb7 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -68,6 +68,12 @@ extern void tick_broadcast_control(enum tick_broadcast_mode mode);
 static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
 #endif /* BROADCAST */
 
+#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
+extern void tick_offline_cpu(unsigned int cpu);
+#else
+static inline void tick_offline_cpu(unsigned int cpu) { }
+#endif
+
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
 #else
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 025f419d16f6..f69ba38573c2 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -844,6 +844,8 @@ static int take_cpu_down(void *_param)
 
 	/* Give up timekeeping duties */
 	tick_handover_do_timer();
+	/* Remove CPU from timer broadcasting */
+	tick_offline_cpu(cpu);
 	/* Park the stopper thread */
 	stop_machine_park(cpu);
 	return 0;
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 5e77662dd2d9..f5490222e134 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -611,6 +611,22 @@ void clockevents_resume(void)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
+
+# ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+/**
+ * tick_offline_cpu - Take CPU out of the broadcast mechanism
+ * @cpu:	The outgoing CPU
+ *
+ * Called on the outgoing CPU after it took itself offline.
+ */
+void tick_offline_cpu(unsigned int cpu)
+{
+	raw_spin_lock(&clockevents_lock);
+	tick_broadcast_offline(cpu);
+	raw_spin_unlock(&clockevents_lock);
+}
+# endif
+
 /**
  * tick_cleanup_dead_cpu - Cleanup the tick and clockevents of a dead cpu
  */
@@ -621,8 +637,6 @@ void tick_cleanup_dead_cpu(int cpu)
 
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
 
-	tick_shutdown_broadcast_oneshot(cpu);
-	tick_shutdown_broadcast(cpu);
 	tick_shutdown(cpu);
 	/*
 	 * Unregister the clock event devices which were
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index ee834d4fb814..a4705191d344 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -36,10 +36,12 @@ static __cacheline_aligned_in_smp DEFINE_RAW_SPINLOCK(tick_broadcast_lock);
 static void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
 static void tick_broadcast_clear_oneshot(int cpu);
 static void tick_resume_broadcast_oneshot(struct clock_event_device *bc);
+static void tick_broadcast_oneshot_offline(unsigned int cpu);
 #else
 static inline void tick_broadcast_setup_oneshot(struct clock_event_device *bc) { BUG(); }
 static inline void tick_broadcast_clear_oneshot(int cpu) { }
 static inline void tick_resume_broadcast_oneshot(struct clock_event_device *bc) { }
+static inline void tick_broadcast_oneshot_offline(unsigned int cpu) { }
 #endif
 
 /*
@@ -444,8 +446,6 @@ void tick_shutdown_broadcast(unsigned int cpu)
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 
 	bc = tick_broadcast_device.evtdev;
-	cpumask_clear_cpu(cpu, tick_broadcast_mask);
-	cpumask_clear_cpu(cpu, tick_broadcast_on);
 
 	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
 		if (bc && cpumask_empty(tick_broadcast_mask))
@@ -454,6 +454,16 @@ void tick_shutdown_broadcast(unsigned int cpu)
 
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
+
+void tick_broadcast_offline(unsigned int cpu)
+{
+	raw_spin_lock(&tick_broadcast_lock);
+	cpumask_clear_cpu(cpu, tick_broadcast_mask);
+	cpumask_clear_cpu(cpu, tick_broadcast_on);
+	tick_broadcast_oneshot_offline(cpu);
+	raw_spin_unlock(&tick_broadcast_lock);
+}
+
 #endif
 
 void tick_suspend_broadcast(void)
@@ -950,14 +960,10 @@ void hotplug_cpu__broadcast_tick_pull(int deadcpu)
 }
 
 /*
- * Remove a dead CPU from broadcasting
+ * Remove a dying CPU from broadcasting
  */
-void tick_shutdown_broadcast_oneshot(unsigned int cpu)
+static void tick_broadcast_oneshot_offline(unsigned int cpu)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
-
 	/*
 	 * Clear the broadcast masks for the dead cpu, but do not stop
 	 * the broadcast device!
@@ -965,8 +971,6 @@ void tick_shutdown_broadcast_oneshot(unsigned int cpu)
 	cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
 	cpumask_clear_cpu(cpu, tick_broadcast_pending_mask);
 	cpumask_clear_cpu(cpu, tick_broadcast_force_mask);
-
-	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 #endif
 
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index e277284c2831..7b2496136729 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -64,7 +64,6 @@ extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt);
 extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu);
 extern void tick_install_broadcast_device(struct clock_event_device *dev);
 extern int tick_is_broadcast_device(struct clock_event_device *dev);
-extern void tick_shutdown_broadcast(unsigned int cpu);
 extern void tick_suspend_broadcast(void);
 extern void tick_resume_broadcast(void);
 extern bool tick_resume_check_broadcast(void);
@@ -78,7 +77,6 @@ static inline void tick_install_broadcast_device(struct clock_event_device *dev)
 static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; }
 static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; }
 static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }
-static inline void tick_shutdown_broadcast(unsigned int cpu) { }
 static inline void tick_suspend_broadcast(void) { }
 static inline void tick_resume_broadcast(void) { }
 static inline bool tick_resume_check_broadcast(void) { return false; }
@@ -128,19 +126,23 @@ static inline int tick_check_oneshot_change(int allow_nohz) { return 0; }
 /* Functions related to oneshot broadcasting */
 #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
 extern void tick_broadcast_switch_to_oneshot(void);
-extern void tick_shutdown_broadcast_oneshot(unsigned int cpu);
 extern int tick_broadcast_oneshot_active(void);
 extern void tick_check_oneshot_broadcast_this_cpu(void);
 bool tick_broadcast_oneshot_available(void);
 extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
 #else /* !(BROADCAST && ONESHOT): */
 static inline void tick_broadcast_switch_to_oneshot(void) { }
-static inline void tick_shutdown_broadcast_oneshot(unsigned int cpu) { }
 static inline int tick_broadcast_oneshot_active(void) { return 0; }
 static inline void tick_check_oneshot_broadcast_this_cpu(void) { }
 static inline bool tick_broadcast_oneshot_available(void) { return tick_oneshot_possible(); }
 #endif /* !(BROADCAST && ONESHOT) */
 
+#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
+extern void tick_broadcast_offline(unsigned int cpu);
+#else
+static inline void tick_broadcast_offline(unsigned int cpu) { }
+#endif
+
 /* NO_HZ_FULL internal */
 #ifdef CONFIG_NO_HZ_FULL
 extern void tick_nohz_init(void);

[tip:timers/core] tick: Remove outgoing CPU from broadcast masks

[tip-bot for Thomas Gleixner]

Commit-ID:  1b72d43237980eab9b6ae6bb8181e51c840377e6
Gitweb:     https://git.kernel.org/tip/1b72d43237980eab9b6ae6bb8181e51c840377e6
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 21 Mar 2019 16:39:20 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 23 Mar 2019 18:26:43 +0100

tick: Remove outgoing CPU from broadcast masks

Valentin reported that unplugging a CPU occasionally results in a warning
in the tick broadcast code which is triggered when an offline CPU is in the
broadcast mask.

This happens because the outgoing CPU is not removing itself from the
broadcast masks, especially not from the broadcast_force_mask. The removal
happens on the control CPU after the outgoing CPU is dead. It's a long
standing issue, but the warning is harmless.

Rework the hotplug mechanism so that the outgoing CPU removes itself from
the broadcast masks after disabling interrupts and removing itself from the
online mask.

Reported-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Valentin Schneider <valentin.schneider@arm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1903211540180.1784@nanos.tec.linutronix.de
---
 include/linux/tick.h         |  6 ++++++
 kernel/cpu.c                 |  2 ++
 kernel/time/clockevents.c    | 18 ++++++++++++++++--
 kernel/time/tick-broadcast.c | 40 +++++++++++++++++++---------------------
 kernel/time/tick-internal.h  | 10 ++++++----
 5 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 55388ab45fd4..76acb48acdb7 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -68,6 +68,12 @@ extern void tick_broadcast_control(enum tick_broadcast_mode mode);
 static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
 #endif /* BROADCAST */
 
+#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
+extern void tick_offline_cpu(unsigned int cpu);
+#else
+static inline void tick_offline_cpu(unsigned int cpu) { }
+#endif
+
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
 #else
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 025f419d16f6..f69ba38573c2 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -844,6 +844,8 @@ static int take_cpu_down(void *_param)
 
 	/* Give up timekeeping duties */
 	tick_handover_do_timer();
+	/* Remove CPU from timer broadcasting */
+	tick_offline_cpu(cpu);
 	/* Park the stopper thread */
 	stop_machine_park(cpu);
 	return 0;
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 5e77662dd2d9..f5490222e134 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -611,6 +611,22 @@ void clockevents_resume(void)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
+
+# ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+/**
+ * tick_offline_cpu - Take CPU out of the broadcast mechanism
+ * @cpu:	The outgoing CPU
+ *
+ * Called on the outgoing CPU after it took itself offline.
+ */
+void tick_offline_cpu(unsigned int cpu)
+{
+	raw_spin_lock(&clockevents_lock);
+	tick_broadcast_offline(cpu);
+	raw_spin_unlock(&clockevents_lock);
+}
+# endif
+
 /**
  * tick_cleanup_dead_cpu - Cleanup the tick and clockevents of a dead cpu
  */
@@ -621,8 +637,6 @@ void tick_cleanup_dead_cpu(int cpu)
 
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
 
-	tick_shutdown_broadcast_oneshot(cpu);
-	tick_shutdown_broadcast(cpu);
 	tick_shutdown(cpu);
 	/*
 	 * Unregister the clock event devices which were
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index ee834d4fb814..0283523de045 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -36,10 +36,12 @@ static __cacheline_aligned_in_smp DEFINE_RAW_SPINLOCK(tick_broadcast_lock);
 static void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
 static void tick_broadcast_clear_oneshot(int cpu);
 static void tick_resume_broadcast_oneshot(struct clock_event_device *bc);
+static void tick_broadcast_oneshot_offline(unsigned int cpu);
 #else
 static inline void tick_broadcast_setup_oneshot(struct clock_event_device *bc) { BUG(); }
 static inline void tick_broadcast_clear_oneshot(int cpu) { }
 static inline void tick_resume_broadcast_oneshot(struct clock_event_device *bc) { }
+static inline void tick_broadcast_oneshot_offline(unsigned int cpu) { }
 #endif
 
 /*
@@ -433,27 +435,29 @@ void tick_set_periodic_handler(struct clock_event_device *dev, int broadcast)
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
-/*
- * Remove a CPU from broadcasting
- */
-void tick_shutdown_broadcast(unsigned int cpu)
+static void tick_shutdown_broadcast(void)
 {
-	struct clock_event_device *bc;
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
-
-	bc = tick_broadcast_device.evtdev;
-	cpumask_clear_cpu(cpu, tick_broadcast_mask);
-	cpumask_clear_cpu(cpu, tick_broadcast_on);
+	struct clock_event_device *bc = tick_broadcast_device.evtdev;
 
 	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
 		if (bc && cpumask_empty(tick_broadcast_mask))
 			clockevents_shutdown(bc);
 	}
+}
 
-	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
+/*
+ * Remove a CPU from broadcasting
+ */
+void tick_broadcast_offline(unsigned int cpu)
+{
+	raw_spin_lock(&tick_broadcast_lock);
+	cpumask_clear_cpu(cpu, tick_broadcast_mask);
+	cpumask_clear_cpu(cpu, tick_broadcast_on);
+	tick_broadcast_oneshot_offline(cpu);
+	tick_shutdown_broadcast();
+	raw_spin_unlock(&tick_broadcast_lock);
 }
+
 #endif
 
 void tick_suspend_broadcast(void)
@@ -950,14 +954,10 @@ void hotplug_cpu__broadcast_tick_pull(int deadcpu)
 }
 
 /*
- * Remove a dead CPU from broadcasting
+ * Remove a dying CPU from broadcasting
  */
-void tick_shutdown_broadcast_oneshot(unsigned int cpu)
+static void tick_broadcast_oneshot_offline(unsigned int cpu)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
-
 	/*
 	 * Clear the broadcast masks for the dead cpu, but do not stop
 	 * the broadcast device!
@@ -965,8 +965,6 @@ void tick_shutdown_broadcast_oneshot(unsigned int cpu)
 	cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
 	cpumask_clear_cpu(cpu, tick_broadcast_pending_mask);
 	cpumask_clear_cpu(cpu, tick_broadcast_force_mask);
-
-	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 #endif
 
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index e277284c2831..7b2496136729 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -64,7 +64,6 @@ extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt);
 extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu);
 extern void tick_install_broadcast_device(struct clock_event_device *dev);
 extern int tick_is_broadcast_device(struct clock_event_device *dev);
-extern void tick_shutdown_broadcast(unsigned int cpu);
 extern void tick_suspend_broadcast(void);
 extern void tick_resume_broadcast(void);
 extern bool tick_resume_check_broadcast(void);
@@ -78,7 +77,6 @@ static inline void tick_install_broadcast_device(struct clock_event_device *dev)
 static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; }
 static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; }
 static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }
-static inline void tick_shutdown_broadcast(unsigned int cpu) { }
 static inline void tick_suspend_broadcast(void) { }
 static inline void tick_resume_broadcast(void) { }
 static inline bool tick_resume_check_broadcast(void) { return false; }
@@ -128,19 +126,23 @@ static inline int tick_check_oneshot_change(int allow_nohz) { return 0; }
 /* Functions related to oneshot broadcasting */
 #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
 extern void tick_broadcast_switch_to_oneshot(void);
-extern void tick_shutdown_broadcast_oneshot(unsigned int cpu);
 extern int tick_broadcast_oneshot_active(void);
 extern void tick_check_oneshot_broadcast_this_cpu(void);
 bool tick_broadcast_oneshot_available(void);
 extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
 #else /* !(BROADCAST && ONESHOT): */
 static inline void tick_broadcast_switch_to_oneshot(void) { }
-static inline void tick_shutdown_broadcast_oneshot(unsigned int cpu) { }
 static inline int tick_broadcast_oneshot_active(void) { return 0; }
 static inline void tick_check_oneshot_broadcast_this_cpu(void) { }
 static inline bool tick_broadcast_oneshot_available(void) { return tick_oneshot_possible(); }
 #endif /* !(BROADCAST && ONESHOT) */
 
+#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_HOTPLUG_CPU)
+extern void tick_broadcast_offline(unsigned int cpu);
+#else
+static inline void tick_broadcast_offline(unsigned int cpu) { }
+#endif
+
 /* NO_HZ_FULL internal */
 #ifdef CONFIG_NO_HZ_FULL
 extern void tick_nohz_init(void);