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

[Valentin Schneider <valentin.schneider@arm.com>]

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);
>