netdev - Re: [PATCH net-next v1] gso: Support partial splitting at the frag

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: <20160824093226.GN3735@gauss.secunet.com>
Date:   Wed, 24 Aug 2016 11:32:27 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
CC:     Netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Alexander Duyck <alexander.h.duyck@...el.com>
Subject: Re: [PATCH net-next v1] gso: Support partial splitting at the
 frag_list pointer

On Tue, Aug 23, 2016 at 07:47:32AM -0700, Alexander Duyck wrote:
> On Mon, Aug 22, 2016 at 10:20 PM, Steffen Klassert
> <steffen.klassert@...unet.com> wrote:
> > Since commit 8a29111c7 ("net: gro: allow to build full sized skb")
> > gro may build buffers with a frag_list. This can hurt forwarding
> > because most NICs can't offload such packets, they need to be
> > segmented in software. This patch splits buffers with a frag_list
> > at the frag_list pointer into buffers that can be TSO offloaded.
> >
> > Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com>
> > ---
> >  net/core/skbuff.c      | 89 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  net/ipv4/af_inet.c     |  7 ++--
> >  net/ipv4/gre_offload.c |  7 +++-
> >  net/ipv4/tcp_offload.c |  3 ++
> >  net/ipv4/udp_offload.c |  9 +++--
> >  net/ipv6/ip6_offload.c |  6 +++-
> >  6 files changed, 114 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 3864b4b6..a614e9d 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3078,6 +3078,92 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> >         sg = !!(features & NETIF_F_SG);
> >         csum = !!can_checksum_protocol(features, proto);
> >
> > +       headroom = skb_headroom(head_skb);
> > +
> > +       if (list_skb && net_gso_ok(features, skb_shinfo(head_skb)->gso_type) &&
> > +           csum && sg && (mss != GSO_BY_FRAGS) &&
> > +           !(features & NETIF_F_GSO_PARTIAL)) {
> 
> Does this really need to be mutually exclusive with
> NETIF_F_GSO_PARTIAL and GSO_BY_FRAGS? 

It should be possible to extend this to NETIF_F_GSO_PARTIAL but
I have no test for this. Regarding GSO_BY_FRAGS, this is rather
new and just used for sctp. I don't know what sctp does with
GSO_BY_FRAGS.

> This is occurring early enough
> that maybe instead of doubling the size of skb_segment you should look
> at instead adding a new static function that could handle splitting
> the frag_list and just call that instead of adding this massive amount
> of code.

Ok, will do that.

> 
> Some of these checks are more expensive than others.  I would
> recommend doing the sg && csum && !(features & NETIF_F_GSO_PARTIAL)
> checks first.  If possible you could even combine some of the checks
> since they are also in the block that sets up partial_segs.  That way
> we can cut down on the total number of conditional branches needed.

We can combine the sg && csum check in the block that sets up partial_segs.
In case this is not NETIF_F_GSO_PARTIAL, I'll do the list_skb and
net_gso_ok() check and call the new static function then.

> >
> > +       if (skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL)
> > +               gso_partial = true;
> > +
> 
> For these kind of blocks it is usually best to just do:
>     gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL);
> 
> The compiler usually does a better job of just doing a bit of
> arithmetic instead of generating a set of test/jump type instructions
> and generally that runs faster since there is less branching.  The
> same applies to all the other cases where you setup gso_partial this
> way.

Good point, I'll change this.

Thanks for the review!