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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

> > > > Would it be better to test skb_is_gso(skb) and only skip fraglist
> > > > GRO
> > > > for HW-GRO skbs, rather than disabling it for all skbs?
> > > 
> > > Yes, for tethering packets, HW-GRO skbs are aggregated by
> > > skb_gro_receive(), while others go through fraglist GRO. However,
> > > care
> > > must be taken to avoid packets arriving out of order. I will work
> > > on
> > > it and submit V4.
> > 
> > Oh right.
> > 
> > As long as the choice is only between fraglist GRO or not fraglist
> > (rather than GRO or bypass GRO), it should not introduce any new
> > reordering concerns.
> > 
> > But the decision cannot be made based on skb_is_gso(skb) of an
> > arriving skb. Because when such a HW-GRO skb arrives a SW GRO context
> > in fraglist mode may already have been opened, and it is too late to
> > convert that to non-fraglist.
> > 
> > So essentially HW-GRO and fraglist are mutually exclusive.
> > 
> > > > 
> > > > If treating the features as mutually exclusive, another option
> > > > would
> > > > be to replace these datapath checks with disabling one at
> > > > configuration
> > > > time, in netdev_fix_features.
> > > > 
> > > 
> > > If V4 work well, there will be no need to check netdev->features.
> > > Linux kernel GRO will be more robust and able to handle scenarios
> > > where both NETIF_F_GRO_HW and NETIF_F_GRO_FRAGLIST are enabled.
> > 
> > What is your plan for v4?
> Based on kernel 7.2.0.rc7 code:
> Add logic in tcp4/6_check_fraglist_gro() as follow:
> if an arriving skb is the first packet in the GRO list.
> 	NAPI_GRO_CB(skb)->is_flist = !sk && !skb_is_gso(skb);
> 
> /*
> * Otherwise, the arriving skb is not the first packet, which means
> * that struct sk_buff *p exists.
> */
> if (!skb_is_gso(skb) || !NAPI_GRO_CB(p)->is_flist) {
> 	/* Aggregate the skb using p's GRO method. */
> 	NAPI_GRO_CB(skb)->is_flist = NAPI_GRO_CB(p)->is_flist;
> } else {
> 	NAPI_GRO_CB(skb)->is_flist = 0;
> 	/* Flush p and start a new GRO list using the non-fraglist
> method. */
> }
> 
> Another code change in
> tcp_gro_receive() {
> 	...
> 	if (unlikely(NAPI_GRO_CB(p)->is_flist)) {
> 		...
> 		/* if aggregate method changed, flush current gro list
> */
> 		flush |= NAPI_GRO_CB(skb)->is_flist != NAPI_GRO_CB(p)-
> >is_flist;
> 		if (flush || skb_gro_receive_list())
> 			...
> 	}
> }
> 
> After this change:
> - NETIF_F_GRO_HW and NETIF_F_GRO_FRAGLIST are no longer mutually
> exclusive.
> - In tethering scenarios, TCP fraglist GRO applies only to consecutive
> non-GSO skbs(!skb_is_gso(skb)).
>   others will adopt skb_gro_receive() path.
> 
> Please provide some suggestions on the changes above. Should I prepare
> V4 patch based on these changes?

Thanks. This sounds good to me.

The risk is that GRO might be less effective at coalescing, if HW-GRO
and non HW-GRO packets alternate regularly.

The alternative to make HW-GRO and fraglist GRO mutually exclusive
does not have that problem, but on the flipside cannot use the fraglist
optimization (esp for forwarding path).

So no free lunch. Either works.