netdev - Re: [RFC PATCH v2 15/21] net/tcp: add MSG

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: <20200727173528.tfsrweswpyjxlqv6@bsd-mbp.dhcp.thefacebook.com>
Date:   Mon, 27 Jul 2020 10:35:28 -0700
From:   Jonathan Lemon <jonathan.lemon@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     netdev <netdev@...r.kernel.org>, kernel-team <kernel-team@...com>,
        Christoph Hellwig <hch@....de>,
        Robin Murphy <robin.murphy@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Maxim Mikityanskiy <maximmi@...lanox.com>,
        bjorn.topel@...el.com, magnus.karlsson@...el.com,
        borisp@...lanox.com, david@...hat.com
Subject: Re: [RFC PATCH v2 15/21] net/tcp: add MSG_NETDMA flag for sendmsg()

On Mon, Jul 27, 2020 at 09:09:48AM -0700, Eric Dumazet wrote:
> On Mon, Jul 27, 2020 at 8:56 AM Jonathan Lemon <jonathan.lemon@...il.com> wrote:
> >
> > On Mon, Jul 27, 2020 at 08:19:43AM -0700, Eric Dumazet wrote:
> > > On Mon, Jul 27, 2020 at 12:51 AM Jonathan Lemon
> > > <jonathan.lemon@...il.com> wrote:
> > > >
> > > > This flag indicates that the attached data is a zero-copy send,
> > > > and the pages should be retrieved from the netgpu module.  The
> > > > socket should should already have been attached to a netgpu queue.
> > > >
> > > > Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
> > > > ---
> > > >  include/linux/socket.h | 1 +
> > > >  net/ipv4/tcp.c         | 8 ++++++++
> > > >  2 files changed, 9 insertions(+)
> > > >
> > > > diff --git a/include/linux/socket.h b/include/linux/socket.h
> > > > index 04d2bc97f497..63816cc25dee 100644
> > > > --- a/include/linux/socket.h
> > > > +++ b/include/linux/socket.h
> > > > @@ -310,6 +310,7 @@ struct ucred {
> > > >                                           */
> > > >
> > > >  #define MSG_ZEROCOPY   0x4000000       /* Use user data in kernel path */
> > > > +#define MSG_NETDMA     0x8000000
> > > >  #define MSG_FASTOPEN   0x20000000      /* Send data in TCP SYN */
> > > >  #define MSG_CMSG_CLOEXEC 0x40000000    /* Set close_on_exec for file
> > > >                                            descriptor received through
> > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > index 261c28ccc8f6..340ce319edc9 100644
> > > > --- a/net/ipv4/tcp.c
> > > > +++ b/net/ipv4/tcp.c
> > > > @@ -1214,6 +1214,14 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
> > > >                         uarg->zerocopy = 0;
> > > >         }
> > > >
> > > > +       if (flags & MSG_NETDMA && size && sock_flag(sk, SOCK_ZEROCOPY)) {
> > > > +               zc = sk->sk_route_caps & NETIF_F_SG;
> > > > +               if (!zc) {
> > > > +                       err = -EFAULT;
> > > > +                       goto out_err;
> > > > +               }
> > > > +       }
> > > >
> > >
> > > Sorry, no, we can not allow adding yet another branch into TCP fast
> > > path for yet another variant of zero copy.
> >
> > I'm not in disagreement with that statement, but the existing zerocopy
> > work makes some assumptions that aren't suitable.  I take it that you'd
> > rather have things folded together so the old/new code works together?
> 
> Exact.  Forcing users to use MSG_NETDMA, yet reusing SOCK_ZEROCOPY is silly.
> 
> SOCK_ZEROCOPY has been added to that user space and kernel would agree
> on MSG_ZEROCOPY being not a nop (as it was on old kernels)
> 
> >
> > Allocating an extra structure for every skbuff isn't ideal in my book.
> >
> 
> We do not allocate a structure for every skbuff. Please look again.

I'm looking here:

    uarg = sock_zerocopy_realloc(sk, size, skb_zcopy(skb));

Doesn't sock_zerocopy_realloc() allocate a new structure if the skb
doesn't have one already?


> > > Overall, I think your patch series desperately tries to add changes in
> > > TCP stack, while there is yet no proof
> > > that you have to use TCP transport between the peers.
> >
> > The goal is having a reliable transport without resorting to RDMA.
> 
> And why should it be TCP ?
> 
> Are you dealing with lost packets, retransmits, timers, and al  ?

Yes?  If there was a true lossless medium, RDMA would have taken over by
now.  Or are you suggesting that the transport protocol reliability
should be performed in userspace?  (not all the world is QUIC yet)
-- 
Jonathan