Re: [PATCH v4] CPU hotplug: active_writer not woken up in some cases - deadlock

[David Hildenbrand <dahi@linux.vnet.ibm.com>]

> On 12/10, David Hildenbrand wrote:
> >
> > @@ -127,20 +119,16 @@ void put_online_cpus(void)
> >  {
> >  	if (cpu_hotplug.active_writer == current)
> >  		return;
> > -	if (!mutex_trylock(&cpu_hotplug.lock)) {
> > -		atomic_inc(&cpu_hotplug.puts_pending);
> > -		cpuhp_lock_release();
> > -		return;
> > -	}
> > -
> > -	if (WARN_ON(!cpu_hotplug.refcount))
> > -		cpu_hotplug.refcount++; /* try to fix things up */
> >  
> > -	if (!--cpu_hotplug.refcount && unlikely(cpu_hotplug.active_writer))
> > -		wake_up_process(cpu_hotplug.active_writer);
> > -	mutex_unlock(&cpu_hotplug.lock);
> > -	cpuhp_lock_release();
> > +	if (atomic_dec_and_test(&cpu_hotplug.refcount) &&
> > +	    waitqueue_active(&cpu_hotplug.wq))
> > +		wake_up(&cpu_hotplug.wq);
> 
> OK, waitqueue_active() looks safe... prepare_to_wait() has a barrier.
> 
> >  void cpu_hotplug_begin(void)
> >  {
> > +	DEFINE_WAIT(wait);
> > +
> >  	cpu_hotplug.active_writer = current;
> >  
> > -	cpuhp_lock_acquire();
> >  	for (;;) {
> > +		cpuhp_lock_acquire();
> 
> not sure I understand why did you move cpuhp_lock_acquire() into
> the loop, but this is minor.

Well I got some lockdep issues and this way I was able to solve them.
(complain about same thread that called cpu_hotplug_begin() calling
put_online_cpus(), so we have to correctly tell lockdep when we get an release
the lock).

So I guess I also need that in the loop, or am I wrong (due to
cpuhp_lock_release())?

> 
> >  		mutex_lock(&cpu_hotplug.lock);
> > -		apply_puts_pending(1);
> > -		if (likely(!cpu_hotplug.refcount))
> > +		prepare_to_wait(&cpu_hotplug.wq, &wait, TASK_UNINTERRUPTIBLE);
> > +		if (likely(!atomic_read(&cpu_hotplug.refcount)))
> >  			break;
> > -		__set_current_state(TASK_UNINTERRUPTIBLE);
> >  		mutex_unlock(&cpu_hotplug.lock);
> > +		cpuhp_lock_release();
> >  		schedule();
> >  	}
> > +
> > +	finish_wait(&cpu_hotplug.wq, &wait);
> >  }
> 
> This is subjective, but how about
> 
> 	static bool xxx(void)
> 	{
> 		mutex_lock(&cpu_hotplug.lock);
> 		if (atomic_read(&cpu_hotplug.refcount) == 0)
> 			return true;
> 		mutex_unlock(&cpu_hotplug.lock);
> 		return false;
> 	}
> 
> 	void cpu_hotplug_begin(void)
> 	{
> 		cpu_hotplug.active_writer = current;
> 
> 		cpuhp_lock_acquire();
> 		wait_event(&cpu_hotplug.wq, xxx());
> 	}
> 
> instead?
> 

What I don't like about that suggestion is that the mutex_lock() happens in
another level of indirection, so by looking at cpu_hotplug_begin() it isn't
obvious that that lock remains locked after this function has been called.

On the other hand this is really a compact one (+ possibly lockdep
annotations) :) .

> Oleg.
> 

It is important that we do the state change to TASK_UNINTERRUPTIBLE prior to
checking for the condition.

Is it guaranteed with wait_event() that things like the following won't happen?

1. CPU1 wakes up the wq (refcount == 0)
2. CPU2 calls get_online_cpus() and increments refcount. (refcount == 1)
2. CPU3 executes xxx() up to "return false;" and gets scheduled away
3. CPU2 calls put_online_cpus(), decrementing the refcount (refcount == 0)
   -> waitqueue not active -> no wake up
4. CPU3 continues executing and sleeps
-> refcount == 0 but writer is not woken up

Saying, does wait_event() take care wakeups while executing xxx()?
(w.g. activating the wait queue, setting TASK_UNINTERRUPTIBLE just before
calling xxx())

In my code, this is guaranteed by calling
prepare_to_wait(&cpu_hotplug.wq, &wait, TASK_UNINTERRUPTIBLE); prior to checking for the condition.

If that is guaranteed, this would work. Will verify that tomorrow.

Thanks a lot!

David