Re: [PATCH net-next v3] gso: Support partial splitting at the frag_list pointer

[Steffen Klassert <steffen.klassert@secunet.com>]

On Mon, Sep 05, 2016 at 10:30:44AM -0700, Alexander Duyck wrote:
> On Mon, Sep 5, 2016 at 3:47 AM, Steffen Klassert
> <steffen.klassert@secunet.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@secunet.com>
> > ---
> >
> > Changes since v1:
> >
> > - Use the assumption that all buffers in the chain excluding the last
> >   containing the same amount of data.
> >
> > - Simplify some checks against gso partial.
> >
> > - Fix the generation of IP IDs.
> >
> > Changes since v2:
> >
> > - Merge common code of gso partial and frag_list pointer splitting.
> >
> >  net/core/skbuff.c      | 50 +++++++++++++++++++++++++++++++++++++++-----------
> >  net/ipv4/af_inet.c     | 14 ++++++++++----
> >  net/ipv4/gre_offload.c |  6 ++++--
> >  net/ipv4/tcp_offload.c | 13 +++++++------
> >  net/ipv4/udp_offload.c |  6 ++++--
> >  net/ipv6/ip6_offload.c |  5 ++++-
> >  6 files changed, 68 insertions(+), 26 deletions(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 3864b4b6..f754d47 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3078,11 +3078,30 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> >         sg = !!(features & NETIF_F_SG);
> >         csum = !!can_checksum_protocol(features, proto);
> >
> > -       /* GSO partial only requires that we trim off any excess that
> > -        * doesn't fit into an MSS sized block, so take care of that
> > -        * now.
> > -        */
> > -       if (sg && csum && (features & NETIF_F_GSO_PARTIAL)) {
> > +       if (sg && csum && (mss != GSO_BY_FRAGS))  {
> > +               if (!(features & NETIF_F_GSO_PARTIAL)) {
> > +                       if (list_skb &&
> > +                           net_gso_ok(features, skb_shinfo(head_skb)->gso_type)) {
> 
> The testing logic here is a bit off.

Yes, apparently :-/

> 
> You need to prevent us from doing the partial_segs bit below if
> NETIF_F_GSO_PARTIAL is not set and if your list_skb or net_gso_ok
> tests fail.  Since as you pointed out we shouldn't ever be trying to
> perform GSO_PARTIAL on a frame that has a frag_list, what you might do
> is something like:
>     if (!list_skb || !net_gso_ok(...))
>         goto normal;
> 
> That way we don't setup partial_segs unless we are actually using it.

Will do that.

> > +
> > +               if (tail->len < gso_size)
> 
> You might swap this around and just setup gso_segs on > gso_size,
> either that or this needs to be "<=" instead of just "<";

Ok, I'll use "<=".