Message-ID: <20220920185021.GA21641@debian.home>
Date:   Tue, 20 Sep 2022 20:50:21 +0200
From:   Guillaume Nault <gnault@...hat.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Benjamin Coddington <bcodding@...hat.com>,
        Trond Myklebust <trondmy@...merspace.com>,
        Scott Mayhew <smayhew@...hat.com>,
        David Miller <davem@...emloft.net>,
        Chuck Lever <chuck.lever@...cle.com>,
        Steve French <sfrench@...ba.org>, Tejun Heo <tj@...nel.org>,
        Anna Schumaker <anna@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Jeff Layton <jlayton@...nel.org>,
        Josef Bacik <josef@...icpanda.com>,
        netdev <netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [RFC net] Should sk_page_frag() also look at the current GFP
 context?

On Mon, Jul 11, 2022 at 05:31:24PM +0200, Eric Dumazet wrote:
> On Mon, Jul 11, 2022 at 4:07 PM Benjamin Coddington <bcodding@...hat.com> wrote:
> >
> > On 8 Jul 2022, at 16:04, Trond Myklebust wrote:
> >
> > > On Fri, 2022-07-08 at 14:10 -0400, Benjamin Coddington wrote:
> > >> On 7 Jul 2022, at 12:29, Eric Dumazet wrote:
> > >>
> > >>> On Fri, Jul 1, 2022 at 8:41 PM Guillaume Nault <gnault@...hat.com>
> > >>> wrote:
> > >>>>
> > >>>> diff --git a/include/net/sock.h b/include/net/sock.h
> > >>>> index 72ca97ccb460..b934c9851058 100644
> > >>>> --- a/include/net/sock.h
> > >>>> +++ b/include/net/sock.h
> > >>>> @@ -46,6 +46,7 @@
> > >>>>  #include <linux/netdevice.h>
> > >>>>  #include <linux/skbuff.h>      /* struct sk_buff */
> > >>>>  #include <linux/mm.h>
> > >>>> +#include <linux/sched/mm.h>
> > >>>>  #include <linux/security.h>
> > >>>>  #include <linux/slab.h>
> > >>>>  #include <linux/uaccess.h>
> > >>>> @@ -2503,14 +2504,17 @@ static inline void
> > >>>> sk_stream_moderate_sndbuf(struct sock *sk)
> > >>>>   * socket operations and end up recursing into sk_page_frag()
> > >>>>   * while it's already in use: explicitly avoid task page_frag
> > >>>>   * usage if the caller is potentially doing any of them.
> > >>>> - * This assumes that page fault handlers use the GFP_NOFS flags.
> > >>>> + * This assumes that page fault handlers use the GFP_NOFS flags
> > >>>> + * or run under memalloc_nofs_save() protection.
> > >>>>   *
> > >>>>   * Return: a per task page_frag if context allows that,
> > >>>>   * otherwise a per socket one.
> > >>>>   */
> > >>>>  static inline struct page_frag *sk_page_frag(struct sock *sk)
> > >>>>  {
> > >>>> -       if ((sk->sk_allocation & (__GFP_DIRECT_RECLAIM |
> > >>>> __GFP_MEMALLOC | __GFP_FS)) ==
> > >>>> +       gfp_t gfp_mask = current_gfp_context(sk->sk_allocation);
> > >>>
> > >>> This is slowing down TCP sendmsg() fast path, reading current-
> > >>>> flags,
> > >>> possibly cold value.
> > >>
> > >> True - current->flags is pretty distant from current->task_frag.
> > >>
> > >>> I would suggest using one bit in sk, close to sk->sk_allocation to
> > >>> make the decision,
> > >>> instead of testing sk->sk_allocation for various flags.
> > >>>
> > >>> Not sure if we have available holes.
> > >>
> > >> Its looking pretty packed on my build.. the nearest hole is 5
> > >> cachelines
> > >> away.
> > >>
> > >> It'd be nice to allow network filesystem to use task_frag when
> > >> possible.
> > >>
> > >> If we expect sk_page_frag() to only return task_frag once per call
> > >> stack,
> > >> then can we simply check it's already in use, perhaps by looking at
> > >> the
> > >> size field?
> > >>
> > >> Or maybe can we set sk_allocation early from current_gfp_context()
> > >> outside
> > >> the fast path?
> > >
> > > Why not just add a bit to sk->sk_allocation itself, and have
> > > __sock_create() default to setting it when the 'kern' parameter is non-
> > > zero? NFS is not alone in following the request of the mm team to
> > > deprecate use of GFP_NOFS and GFP_NOIO.
> >
> > Can we overload sk_allocation safely?  There's 28 GFP flags already, I'm
> > worried about unintended consequences if sk_allocation gets passed on.
> >
> > What about a flag in sk_gso_type?  Looks like there's 13 free there, and its
> > in the same cacheline as sk_allocation and sk_frag.
> 
> I think we could overload GFP_COMP with little risk.

Reviving this semi-old thread after discussions at LPC.

It seems we won't have a clear path for how to make sk_page_frag()
aware of memalloc_nofs or memalloc_noio contexts before some time.
Since we continue to get bug reports for this issue, I'm thinking of
just setting sk_allocation to GFP_NOFS for sunrpc sockets. Then we'll
can think of a better way of removing GFP_NOFS or GFP_NOIO from the
different networking file systems and block devices.

Here's a summary of all the long term options discussed so far.

1) Use a special bit in sk_allocation.

Either repurpose an existing bit like GFP_COMP or allocate a free one.
Then sk_page_frag() could test this bit and avoid returning
current->task_frag when it's set. That bit would have to be masked
every time sk_allocation is used to allocate memory.
Overall, it looks a bit like using GFP_NOFS with a different name,
apart that it allows the socket to allocate memory with GFP_KERNEL when
not in memalloc_nofs critical sections (but I'm not sure if it's a
practical gain for NFS).

Alternatively, there's a one bit hole in struct sock_common, right
after skc_state, which could be used to store a 'skc_no_task_frag'
flag (the cache line should be hot because of skc_state). Any socket
user that could be called during memory reclaim could set this bit to
prevent sk_page_frag() from using current->taskfrag.

2) Avoid using current->task_frag for kernel sockets.

Since sk_kern_sock is in the same cache line as sk_allocation, we
probably could let sk_page_frag() test if sk is a kernel socket and
avoid using current->task_frag in this case. Alternatively, we could
define a new flag as in option 1 and automatically set it when creating
kernel sockets (as proposed by Trond).

However, there are many kernel socket users and, so far, only NFS (and
maybe other networking FS in the future) need this protection. So it
looks like a pretty big hammer. Also, NBD uses sockets passed from user
space. Therefore if it were to phase out GFP_NOFS, sk_page_frag() could
return current->task_frag again (maybe NBD should transfrom its sockets
to kernel sockets, but that's a bit of a tangential discussion).

3) Adjust sk_allocation when necessary.

The idea would be to update sk_allocation before entering TCP fast
path. Something like:

  old_sk_allocation = sk->sk_allocation;
  sk->sk_allocation = current_gfp_context(sk->sk_allocation);
  ... use sk ...
  sk->sk_allocation = old_sk_allocation;

That doesn't seem feasible though, as this assumes exclusive access to
the socket, but we grab the socket lock only after entering
tcp_sendmsg().

A similar idea was to do this automatically in kernel_sendmsg(), but
it faces the same exclusive access problem. Furthermore, not all
GFP_NOFS users use kernel_sendmsg() (same problem as with option 2).

4) Detect if current->task_frag is already in use.

There may be a way for sk_page_frag() to detect if current->task_frag
is already in use, that is, if it's in a recursive call. That'd be nice
as that'd avoid the need for any heuristic based on sk_allocation.
However, I can't think of any way to do that efficiently and without
adding bits in current.

Thanks to all those involved in this thread, and to Paolo and Eric for
the fruitful discussions at LPC.

Powered by blists - more mailing lists