[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zw7Un-Cr8JA4oMv0@google.com>
Date: Tue, 15 Oct 2024 16:46:23 -0400
From: Brian Geffon <bgeffon@...gle.com>
To: brauner@...nel.org
Cc: bristot@...hat.com, bsegall@...gle.com, cmllamas@...gle.com,
dietmar.eggemann@....com, ebiggers@...nel.org, jack@...e.cz,
jing.xia@...soc.com, juri.lelli@...hat.com, ke.wang@...soc.com,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
mgorman@...e.de, mingo@...hat.com, peterz@...radead.org,
rostedt@...dmis.org, vincent.guittot@...aro.org,
viro@...iv.linux.org.uk, vschneid@...hat.com,
xuewen.yan94@...il.com, xuewen.yan@...soc.com,
Benoit Lize <lizeb@...gle.com>
Subject: Re: [RFC PATCH] epoll: Add synchronous wakeup support for
ep_poll_callback
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@...gle.com>
> Tested-by: Brian Geffon <bgeffon@...gle.com>
> Reported-by: Benoit Lize <lizeb@...gle.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@...gle.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@...soc.com>
> > > Signed-off-by: Jing Xia <jing.xia@...soc.com>
> > > Signed-off-by: Xuewen Yan <xuewen.yan@...soc.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
> > >
> >
Powered by blists - more mailing lists