Re: [PATCH v1] CPU hotplug: active_reader not woken up in some cases - deadlock

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, Dec 08, 2014 at 07:13:03PM +0100, David Hildenbrand wrote:
> Commit b2c4623dcd07 ("rcu: More on deadlock between CPU hotplug and expedited
> grace periods") introduced another problem that can easily be reproduced by
> starting/stopping cpus in a loop.
> 
> E.g.:
>   for i in `seq 5000`; do
>       echo 1 > /sys/devices/system/cpu/cpu1/online
>       echo 0 > /sys/devices/system/cpu/cpu1/online
>   done
> 
> Will result in:
>   INFO: task /cpu_start_stop:1 blocked for more than 120 seconds.
>   Call Trace:
>   ([<00000000006a028e>] __schedule+0x406/0x91c)
>    [<0000000000130f60>] cpu_hotplug_begin+0xd0/0xd4
>    [<0000000000130ff6>] _cpu_up+0x3e/0x1c4
>    [<0000000000131232>] cpu_up+0xb6/0xd4
>    [<00000000004a5720>] device_online+0x80/0xc0
>    [<00000000004a57f0>] online_store+0x90/0xb0
>   ...
> 
> And a deadlock.
> 
> Problem is that if the last ref in put_online_cpus() can't get the
> cpu_hotplug.lock the puts_pending count is incremented, but a sleeping active_writer
> might never be woken up, therefore never exiting the loop in cpu_hotplug_begin().
> 
> This quick fix wakes up the active_writer proactively. The writer already
> goes back to sleep if the ref count isn't already down to 0, so this should be
> fine.
> 
> Can't reproduce the error with this fix.

Good catch!

But don't we need to use exactly the same value for the NULL check
and for the wakeup?  Otherwise, wouldn't it be possible for
cpu_hotplug.active_writer to be non-NULL for the check but NULL
for the wake_up_process()?

							Thanx, Paul

> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
>  kernel/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 90a3d01..e77740583 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -117,6 +117,9 @@ void put_online_cpus(void)
>  		return;
>  	if (!mutex_trylock(&cpu_hotplug.lock)) {
>  		atomic_inc(&cpu_hotplug.puts_pending);
> +		/* we might be the last one */
> +		if (unlikely(cpu_hotplug.active_writer))
> +			wake_up_process(cpu_hotplug.active_writer);
>  		cpuhp_lock_release();
>  		return;
>  	}
> -- 
> 1.8.5.5
>