On Thu, Nov 24, 2022 at 12:11:23AM +0000, Rishabh Bhatnagar wrote: > From: Soheil Hassas Yeganeh <soheil@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxx > > 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@xxxxxxxxx > Link: https://lkml.kernel.org/r/CAHk-=wizk=OxUyQPbO8MS41w2Pag1kniUV5WdD5qWL-gq1kjDA@xxxxxxxxxxxxxx > Link: https://lkml.kernel.org/r/20201106231635.3528496-2-soheil.kdev@xxxxxxxxx > Fixes: abc610e01c66 ("fs/epoll: avoid barrier after an epoll_wait(2) timeout") > Signed-off-by: Soheil Hassas Yeganeh <soheil@xxxxxxxxxx> > Tested-by: Guantao Liu <guantaol@xxxxxxxxxx> > Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > Reported-by: Guantao Liu <guantaol@xxxxxxxxxx> > Reviewed-by: Eric Dumazet <edumazet@xxxxxxxxxx> > Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx> > Reviewed-by: Khazhismel Kumykov <khazhy@xxxxxxxxxx> > Reviewed-by: Davidlohr Bueso <dbueso@xxxxxxx> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Rishabh Bhatnagar <risbhat@xxxxxxxxxx> Acked-by: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxx> > --- > 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 >
