[PATCH net v4] net: gro: Fix nesting of TCP GSO SKBs in skb_gro_receive_list()

[Willem de Bruijn]

> > > @@ -395,12 +396,20 @@ static void tcp4_check_fraglist_gro(struct
> > > list_head *head, struct sk_buff *skb,
> > >         struct net *net;
> > >         int iif, sdif;
> > >
> > > -       if (likely(!(skb->dev->features & NETIF_F_GRO_FRAGLIST)))
> > > -               return;
> > > -
> > >         p = tcp_gro_lookup(head, th);
> > >         if (p) {
> > > -               NAPI_GRO_CB(skb)->is_flist = NAPI_GRO_CB(p)-
> > > >is_flist;
> > > +               /* flist GRO applies to consecutive non-GSO skbs */
> > > +               if (!skb_is_gso(skb) || !NAPI_GRO_CB(p)->is_flist)
> > > {
> > > +                       NAPI_GRO_CB(skb)->is_flist =
> > > NAPI_GRO_CB(p)->is_flist;
> > > +                       return;
> > > +               }
> > > +
> > > +               /* Fall back to the regular GRO path */
> > > +               if (NAPI_GRO_CB(p)->count == 1)
> > > +                       NAPI_GRO_CB(p)->is_flist = 0;
> > > +
> > > +               NAPI_GRO_CB(skb)->is_flist = 0;
> > > +
> >
> > This reduces the chance to have to flush, by downgrading fraglist GRO
> > packets if a hardware GRO packet is detected at the stage (count ==
> > 1)
> > where fraglist is not enabled in practice.
> >
> > This is quite a bit of complexity. And if hardware GRO is enabled,
> > what
> > are the odds in practice that the device does not produce any
> > hardware
> > GRO packets, yet the software GRO stack is capable of assembling
> > signficant GSO packets. I suspect low.
> >
> > The solution is technically correct. But would simply disabling
> > fraglist
> > GRO when hardware GRO is enabled not be simpler? Or only keeping
> > the flush and avoiding this micro-optimization.
>
> In V2, for the tethering scenario, we directly flush GSO packets.
> This is the simplest way to address the reported issue,
> but those packets can no longer be aggregated by regular GRO.
> This is the main difference from V4 and Jakub pointed out this issue.
>
> V3 disables fraglist GRO when hardware GRO is enabled.
> The additional suggestion for V3 is “replace these
> datapath checks by disabling it at configuration time in
> netdev_fix_features.”
> However, if we disable NETIF_F_GRO_FRAGLIST in netdev_fix_features,
> UDP packets will also can't aggregate by fraglist. This will affect
> UDP performance.
>
> V4 is more complex than V2, but it can maximize the number of
> packets aggregated by GRO. It works similarly to how regular
> GRO tries to further aggregate TCP GSO packets.
>
> This is a trade-off between code complexity and
> the GRO aggregation rate. You can choose either approach.
> If you have another solution in mind, please let me know.

So this code is there only to allow UDP to use fraglist software
aggregation mode while HW-GRO handles TCP aggregation
And fraglist aggregation is only really useful in the forwarding
case.

The benefit over non-fraglist aggregation is moderate. I'm
a skeptical that maintaining extra code for this edge case
targets a significant workload and is worth it. Open to other
opinions. But i'd just fall back onto non-fraglist broadly.

If keeping the optimization, the patch needs to be respun with
both the commit-msg clarifications, and a Link to this discussion.