netdev - Re: [PATCH v2] fs/epoll: Enable non-blocking busypoll when epoll timeout is 0

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] [day] [month] [year] [list]

Date:   Mon, 6 Jul 2020 15:57:21 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     "Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Cc:     linux-fsdevel@...r.kernel.org, Netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH v2] fs/epoll: Enable non-blocking busypoll when epoll
 timeout is 0

On Mon, Jul 6, 2020 at 3:33 PM Samudrala, Sridhar
<sridhar.samudrala@...el.com> wrote:
>
>
>
> On 7/6/2020 1:36 PM, Alexander Duyck wrote:
> > On Wed, Jun 24, 2020 at 4:03 PM Sridhar Samudrala
> > <sridhar.samudrala@...el.com> wrote:
> >>
> >> This patch triggers non-blocking busy poll when busy_poll is enabled,
> >> epoll is called with a timeout of 0 and is associated with a napi_id.
> >> This enables an app thread to go through napi poll routine once by
> >> calling epoll with a 0 timeout.
> >>
> >> poll/select with a 0 timeout behave in a similar manner.
> >>
> >> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
> >>
> >> v2:
> >> Added net_busy_loop_on() check (Eric)
> >>
> >> ---
> >>   fs/eventpoll.c | 13 +++++++++++++
> >>   1 file changed, 13 insertions(+)
> >>
> >> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> >> index 12eebcdea9c8..c33cc98d3848 100644
> >> --- a/fs/eventpoll.c
> >> +++ b/fs/eventpoll.c
> >> @@ -1847,6 +1847,19 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
> >>                  eavail = ep_events_available(ep);
> >>                  write_unlock_irq(&ep->lock);
> >>
> >> +               /*
> >> +                * Trigger non-blocking busy poll if timeout is 0 and there are
> >> +                * no events available. Passing timed_out(1) to ep_busy_loop
> >> +                * will make sure that busy polling is triggered only once.
> >> +                */
> >> +               if (!eavail && net_busy_loop_on()) {
> >> +                       ep_busy_loop(ep, timed_out);
> >> +                       write_lock_irq(&ep->lock);
> >> +                       eavail = ep_events_available(ep);
> >> +                       write_unlock_irq(&ep->lock);
> >> +               }
> >> +
> >>                  goto send_events;
> >>          }
> >
> > Doesn't this create a scenario where the NAPI ID will not be
> > disassociated if the polling fails?
> >
> > It seems like in order to keep parity with existing busy poll code you
> > should need to check for !eavail after you release the lock and if
> > that is true you should be calling ep_reset_busy_poll_napi_id so that
> > you disassociate the NAPI ID from the eventpoll.
>
> We are not going to sleep in this code path. I think napi id needs to be
> reset only if we are going to sleep and a wakeup is expected to set the
> nap_id again.

That wasn't my understanding of how that worked. Basically the whole
point of clearing the napi ID from the eventpoll instance is to handle
the fact that the NAPI instance may no longer be routing packets to
us. So when the NAPI instance failed to provide us with a packet when
we made a call to busy poll we would clear the napi_id which would
then allow a new napi_id to be assigned. Otherwise what you end up
with is an eventpoll with a static napi_id value since it will never
be cleared or updated once it is set, assuming all the calls to this
function have a timeout of 0 in your test case.

The problem is there isn't a 1:1 mapping between eventpolls and
sockets or NAPI IDs. It is a many:1 and as a result we have queues go
idle while others become busy so we need a way to shift from an idle
one to a busy one in the event of the traffic shifting from one port
to another.