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@xxxxxxxxxx> > Tested-by: Brian Geffon <bgeffon@xxxxxxxxxx> > Reported-by: Benoit Lize <lizeb@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > Signed-off-by: Jing Xia <jing.xia@xxxxxxxxxx> > > > Signed-off-by: Xuewen Yan <xuewen.yan@xxxxxxxxxx> > > > --- > > > 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 > > > > >
