Messages in this thread Patch in this message | | | Date | Tue, 10 Dec 2019 08:29:21 +0100 | From | Ingo Molnar <> | Subject | Re: [RFC PATCH] sched/wait: Make interruptible exclusive waitqueue wakeups reliable |
| |
* Oleg Nesterov <oleg@redhat.com> wrote:
> > 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.
Yeah, indeed - it assumes that the condition is stable from wakeup to wakee running - which as Linus said it must be, because otherwise exclusive waiters couldn't reliably exit the wait loop.
So there's no bug. How about the clarifying comment below?
Thanks,
Ingo
kernel/sched/wait.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c index ba059fbfc53a..6783bac00b5c 100644 --- a/kernel/sched/wait.c +++ b/kernel/sched/wait.c @@ -290,6 +290,11 @@ long prepare_to_wait_event(struct wait_queue_head *wq_head, struct wait_queue_en * 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. + * + * In other words, if an exclusive waiter got here, then the + * waitqueue condition is and stays true and we are guaranteed + * to exit the waitqueue loop and will ignore the -ERESTARTSYS + * and return success. */ list_del_init(&wq_entry->entry); ret = -ERESTARTSYS;
| |