Messages in this thread | | | Date | Mon, 9 Dec 2019 13:08:53 +0100 | From | Oleg Nesterov <> | Subject | Re: [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.
| |