lkml.org 
[lkml]   [2019]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [WARNING] tick_handle_oneshot_broadcast() on !online CPU
From
Date
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);
>

\
 
 \ /
  Last update: 2019-03-21 17:31    [W:0.635 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site