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

quoted

quoted

@@ -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)-

quoted

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.