lkml.org 
[lkml]   [2022]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 2/2] epoll: check for events when removing a timed out thread from the wait queue
    On Thu, Nov 24, 2022 at 12:11:23AM +0000, Rishabh Bhatnagar wrote:
    > From: Soheil Hassas Yeganeh <soheil@google.com>
    >
    > Commit 289caf5d8f6c61c6d2b7fd752a7f483cd153f182 upstream.
    >
    > Patch series "simplify ep_poll".
    >
    > This patch series is a followup based on the suggestions and feedback by
    > Linus:
    > https://lkml.kernel.org/r/CAHk-=wizk=OxUyQPbO8MS41w2Pag1kniUV5WdD5qWL-gq1kjDA@mail.gmail.com
    >
    > The first patch in the series is a fix for the epoll race in presence of
    > timeouts, so that it can be cleanly backported to all affected stable
    > kernels.
    >
    > The rest of the patch series simplify the ep_poll() implementation. Some
    > of these simplifications result in minor performance enhancements as well.
    > We have kept these changes under self tests and internal benchmarks for a
    > few days, and there are minor (1-2%) performance enhancements as a result.
    >
    > This patch (of 8):
    >
    > After abc610e01c66 ("fs/epoll: avoid barrier after an epoll_wait(2)
    > timeout"), we break out of the ep_poll loop upon timeout, without checking
    > whether there is any new events available. Prior to that patch-series we
    > always called ep_events_available() after exiting the loop.
    >
    > This can cause races and missed wakeups. For example, consider the
    > following scenario reported by Guantao Liu:
    >
    > Suppose we have an eventfd added using EPOLLET to an epollfd.
    >
    > Thread 1: Sleeps for just below 5ms and then writes to an eventfd.
    > Thread 2: Calls epoll_wait with a timeout of 5 ms. If it sees an
    > event of the eventfd, it will write back on that fd.
    > Thread 3: Calls epoll_wait with a negative timeout.
    >
    > Prior to abc610e01c66, it is guaranteed that Thread 3 will wake up either
    > by Thread 1 or Thread 2. After abc610e01c66, Thread 3 can be blocked
    > indefinitely if Thread 2 sees a timeout right before the write to the
    > eventfd by Thread 1. Thread 2 will be woken up from
    > schedule_hrtimeout_range and, with evail 0, it will not call
    > ep_send_events().
    >
    > To fix this issue:
    > 1) Simplify the timed_out case as suggested by Linus.
    > 2) while holding the lock, recheck whether the thread was woken up
    > after its time out has reached.
    >
    > Note that (2) is different from Linus' original suggestion: It do not set
    > "eavail = ep_events_available(ep)" to avoid unnecessary contention (when
    > there are too many timed-out threads and a small number of events), as
    > well as races mentioned in the discussion thread.
    >
    > This is the first patch in the series so that the backport to stable
    > releases is straightforward.
    >
    > Link: https://lkml.kernel.org/r/20201106231635.3528496-1-soheil.kdev@gmail.com
    > Link: https://lkml.kernel.org/r/CAHk-=wizk=OxUyQPbO8MS41w2Pag1kniUV5WdD5qWL-gq1kjDA@mail.gmail.com
    > Link: https://lkml.kernel.org/r/20201106231635.3528496-2-soheil.kdev@gmail.com
    > Fixes: abc610e01c66 ("fs/epoll: avoid barrier after an epoll_wait(2) timeout")
    > Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
    > Tested-by: Guantao Liu <guantaol@google.com>
    > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
    > Reported-by: Guantao Liu <guantaol@google.com>
    > Reviewed-by: Eric Dumazet <edumazet@google.com>
    > Reviewed-by: Willem de Bruijn <willemb@google.com>
    > Reviewed-by: Khazhismel Kumykov <khazhy@google.com>
    > Reviewed-by: Davidlohr Bueso <dbueso@suse.de>
    > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    > Signed-off-by: Rishabh Bhatnagar <risbhat@amazon.com>

    Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

    > ---
    > fs/eventpoll.c | 25 ++++++++++++++++---------
    > 1 file changed, 16 insertions(+), 9 deletions(-)
    >
    > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
    > index e5496483a882..877f9f61a4e8 100644
    > --- a/fs/eventpoll.c
    > +++ b/fs/eventpoll.c
    > @@ -1928,23 +1928,30 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
    > }
    > write_unlock_irq(&ep->lock);
    >
    > - if (eavail || res)
    > - break;
    > + if (!eavail && !res)
    > + timed_out = !schedule_hrtimeout_range(to, slack,
    > + HRTIMER_MODE_ABS);
    >
    > - if (!schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS)) {
    > - timed_out = 1;
    > - break;
    > - }
    > -
    > - /* We were woken up, thus go and try to harvest some events */
    > + /*
    > + * We were woken up, thus go and try to harvest some events.
    > + * If timed out and still on the wait queue, recheck eavail
    > + * carefully under lock, below.
    > + */
    > eavail = 1;
    > -
    > } while (0);
    >
    > __set_current_state(TASK_RUNNING);
    >
    > if (!list_empty_careful(&wait.entry)) {
    > write_lock_irq(&ep->lock);
    > + /*
    > + * If the thread timed out and is not on the wait queue, it
    > + * means that the thread was woken up after its timeout expired
    > + * before it could reacquire the lock. Thus, when wait.entry is
    > + * empty, it needs to harvest events.
    > + */
    > + if (timed_out)
    > + eavail = list_empty(&wait.entry);
    > __remove_wait_queue(&ep->wq, &wait);
    > write_unlock_irq(&ep->lock);
    > }
    > --
    > 2.37.1
    >

    \
     
     \ /
      Last update: 2022-11-24 08:50    [W:2.770 / U:0.644 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site