linux-kernel - Re: [RFC PATCH] epoll: Add synchronous wakeup support for ep_poll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADyq12w2KRUZCu0hLA8TJH-e+766Jq_vG9SDYtDBYXzR=r9wvg@mail.gmail.com>
Date: Wed, 25 Sep 2024 14:38:02 -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

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>


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
> >
>