Re: [RFC PATCH] epoll: Add synchronous wakeup support for ep_poll_callback

[Brian Geffon <bgeffon@google.com>]

On Wed, Sep 25, 2024 at 02:38:02PM -0400, Brian Geffon wrote:
> I think this patch really needs help with the commit message, something like:
> 
> wait_queue_func_t accepts 4 arguments (struct wait_queue_entry
> *wq_entry, unsigned mode, int flags, void *key);
> 
> In the case of poll and select the wait queue function is pollwake in
> fs/select.c, this wake function passes
> the third argument flags as the sync parameter to the
> default_wake_function defined in kernel/sched/core.c. This
> argument is passed along to try_to_wake_up which continues to pass
> down the wake flags to select_task_rq and finally
> in the case of CFS select_task_rq_fair. In select_task_rq_fair the
> sync flag is passed down to the wake_affine_* functions
> in kernel/sched/fair.c which accept and honor the sync flag.
> 
> Epoll however when reciving the WF_SYNC flag completely drops it on
> the floor, the wakeup function used
> by epoll is defined in fs/eventpoll.c, ep_poll_callback. This callback
> receives a sync flag just like pollwake;
> however, it never does anything with it. Ultimately it wakes up the
> waiting task directly using wake_up.
> 
> This shows that there seems to be a divergence between poll/select and
> epoll regarding honoring sync wakeups.
> 
> I have tested this patch through self tests and numerous runs of the
> perf benchmarks for epoll. All tests past and
> I did not see any observable performance changes in epoll_wait.
> 
> Reviewed-by: Brian Geffon <bgeffon@google.com>
> Tested-by: Brian Geffon <bgeffon@google.com>
> Reported-by: Benoit Lize <lizeb@google.com>

Friendly ping on this. Would someone mind taking a look and picking this
up?

> 
> 
> On Thu, Sep 19, 2024 at 8:36 AM Brian Geffon <bgeffon@google.com> wrote:
> >
> > We've also observed this issue on ChromeOS, it seems like it might long-standing epoll bug as it diverges from the behavior of poll. Any chance a maintainer can take a look?
> >
> > Thanks
> > Brian
> >
> > On Fri, Apr 26, 2024 at 04:05:48PM +0800, Xuewen Yan wrote:
> > > Now, the epoll only use wake_up() interface to wake up task.
> > > However, sometimes, there are epoll users which want to use
> > > the synchronous wakeup flag to hint the scheduler, such as
> > > Android binder driver.
> > > So add a wake_up_sync() define, and use the wake_up_sync()
> > > when the sync is true in ep_poll_callback().
> > >
> > > Co-developed-by: Jing Xia <jing.xia@unisoc.com>
> > > Signed-off-by: Jing Xia <jing.xia@unisoc.com>
> > > Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
> > > ---
> > >  fs/eventpoll.c       | 5 ++++-
> > >  include/linux/wait.h | 1 +
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > > index 882b89edc52a..9b815e0a1ac5 100644
> > > --- a/fs/eventpoll.c
> > > +++ b/fs/eventpoll.c
> > > @@ -1336,7 +1336,10 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
> > >                               break;
> > >                       }
> > >               }
> > > -             wake_up(&ep->wq);
> > > +             if (sync)
> > > +                     wake_up_sync(&ep->wq);
> > > +             else
> > > +                     wake_up(&ep->wq);
> > >       }
> > >       if (waitqueue_active(&ep->poll_wait))
> > >               pwake++;
> > > diff --git a/include/linux/wait.h b/include/linux/wait.h
> > > index 8aa3372f21a0..2b322a9b88a2 100644
> > > --- a/include/linux/wait.h
> > > +++ b/include/linux/wait.h
> > > @@ -221,6 +221,7 @@ void __wake_up_pollfree(struct wait_queue_head *wq_head);
> > >  #define wake_up_all(x)                       __wake_up(x, TASK_NORMAL, 0, NULL)
> > >  #define wake_up_locked(x)            __wake_up_locked((x), TASK_NORMAL, 1)
> > >  #define wake_up_all_locked(x)                __wake_up_locked((x), TASK_NORMAL, 0)
> > > +#define wake_up_sync(x)                      __wake_up_sync(x, TASK_NORMAL)
> > >
> > >  #define wake_up_interruptible(x)     __wake_up(x, TASK_INTERRUPTIBLE, 1, NULL)
> > >  #define wake_up_interruptible_nr(x, nr)      __wake_up(x, TASK_INTERRUPTIBLE, nr, NULL)
> > > --
> > > 2.25.1
> > >
> >