lkml.org 
[lkml]   [2014]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4] CPU hotplug: active_writer not woken up in some cases - deadlock
> 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



\
 
 \ /
  Last update: 2014-12-10 21:01    [W:0.241 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site