lkml.org 
[lkml]   [2019]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] sched/wait: Make interruptible exclusive waitqueue wakeups reliable
On 12/09, Ingo Molnar wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > The reason it is buggy is that wait_event_interruptible_exclusive()
> > does this (inside the __wait_event() macro that it expands to):
> >
> > long __int = prepare_to_wait_event(&wq_head, &__wq_entry, state);
> >
> > if (condition)
> > break;
> > if (___wait_is_interruptible(state) && __int) {
> > __ret = __int;
> > goto __out;
> >
> > and the thing is, if does that "__ret = __int" case and returns
> > -ERESTARTSYS,

But note that it checks "condition" after prepare_to_wait_event(), if it is
true then ___wait_is_interruptible() won't be even called.

> it's possible that the wakeup event has already been
> > consumed, because we've added ourselves as an exclusive writer to the
> > queue. So it _says_ it was interrupted, not woken up, and the wait got
> > cancelled, but because we were an exclusive waiter, we might be the
> > _only_ thing that got woken up, and the wakeup basically got forgotten
> > - all the other exclusive waiters will remain waiting.
>
> So the place that detects interruption is prepare_to_wait_event():

Yes,

> long prepare_to_wait_event(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry, int state)
> {
> unsigned long flags;
> long ret = 0;
>
> spin_lock_irqsave(&wq_head->lock, flags);
> if (signal_pending_state(state, current)) {
> /*
> * Exclusive waiter must not fail if it was selected by wakeup,
> * it should "consume" the condition we were waiting for.
> *
> * The caller will recheck the condition and return success if
> * we were already woken up, we can not miss the event because
> * wakeup locks/unlocks the same wq_head->lock.
> *
> * But we need to ensure that set-condition + wakeup after that
> * can't see us, it should wake up another exclusive waiter if
> * we fail.
> */
> list_del_init(&wq_entry->entry);
> ret = -ERESTARTSYS;

...

> I think we can indeed lose an exclusive event here, despite the comment
> that argues that we shouldn't: if we were already removed from the list

If we were already removed from the list and condition is true, we can't
miss it, ret = -ERESTARTSYS won't be used. This is what this part of the
comment above

* The caller will recheck the condition and return success if
* we were already woken up, we can not miss the event because
* wakeup locks/unlocks the same wq_head->lock.

tries to explain.

> then list_del_init() does nothing and loses the exclusive event AFAICS.

list_del_init() ensures that wake_up() can't pick this task after
prepare_to_wait_event() returns.

IOW. Suppose that ___wait_event() races with

condition = true;
wake_up();

if wake_up() happens before prepare_to_wait_event(), __wait_event() will
see condition == true, -ERESTARTSYS returned by prepare_to_wait_event() has
no effect.

If wake_up() comes after prepare_to_wait_event(), the task was already removed
from the list, another exclusive waiter (if any) will be woken up. In this case
__wait_event() can return success or -ERESTARTSYS, both are correct.

No?

Oleg.

\
 
 \ /
  Last update: 2019-12-09 13:09    [W:1.244 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site