Re: Reorganising how the networking layer handles memory

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

David Howells wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Tue, 06 May 2025 14:50:49 +0100 David Howells wrote:
> > > Jakub Kicinski <kuba@kernel.org> wrote:
> > > > > (2) sendmsg(MSG_ZEROCOPY) suffers from the O_DIRECT vs fork() bug because
> > > > >      it doesn't use page pinning.  It needs to use the GUP routines.  
> > > > 
> > > > We end up calling iov_iter_get_pages2(). Is it not setting
> > > > FOLL_PIN is a conscious choice, or nobody cared until now?  
> > > 
> > > iov_iter_get_pages*() predates GUP, I think.  There's now an
> > > iov_iter_extract_pages() that does the pinning stuff, but you have to do a
> > > different cleanup, which is why I created a new API call.
> > > 
> > > iov_iter_extract_pages() also does no pinning at all on pages extracted from a
> > > non-user iterator (e.g. ITER_BVEC).
> > 
> > FWIW it occurred to me after hitting send that we may not care. 
> > We're talking about Tx, so the user pages are read only for the kernel.
> > I don't think we have the "child gets the read data" problem?
> 
> Worse: if the child alters the data in the buffer to be transmitted after the
> fork() (say it calls free() and malloc()), it can do so; if the parent tries
> that, there will be no effect.
> 
> > Likely all this will work well for ZC but not sure if we can "convert"
> > the stack to phyaddr+len.
> 
> Me neither.  We also use bio_vec[] to hold lists of memory and then trawl them
> to do cleanup, but a conversion to holding {phys,len} will mandate being able
> to do some sort of reverse lookup.
> 
> > Okay, just keep in mind that we are working on 800Gbps NIC support these
> > days, and MTU does not grow. So whatever we do - it must be fast fast.
> 
> Crazy:-)
> 
> One thing I've noticed in the uring stuff is that it doesn't seem to like the
> idea of having an sk_buff pointing to more than one ubuf_info, presumably
> because the sk_buff will point to the ubuf_info holding the zerocopyable data.
> Is that actually necessary for SOCK_STREAM, though?

In MSG_ZEROCOPY this limitation of at most one ubuf_info per skb was
chosen just because it was simpler and sufficient.

A single skb can still combine skb frags from multiple consecutive
sendmsg requests, including multiple separate MSG_ZEROCOPY calls.
Because the ubuf_info notification is for a range of bytes.

There is a rare edge case in skb_zerocopy_iter_stream that detects
two ubuf_infos on a single skb.

                /* An skb can only point to one uarg. This edge case happens
                 * when TCP appends to an skb, but zerocopy_realloc triggered
                 * a new alloc.
                 */     
                if (orig_uarg && uarg != orig_uarg)
                        return -EEXIST;

Instead TCP then just creates a new skb.
This will result in smaller skbs than otherwise. But as said is rare.

> My thought for SOCK_STREAM is to have an ordered list of zerocopy source
> records on the socket and a completion counter and not tag the skbuffs at all.
> That way, an skbuff can carry data for multiple zerocopy send requests.
> 
> David
>