Re: [PATCH net-next v4 3/5] net: gso: support re-segmentation of TCP GSO skbs

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

Wang Zhan wrote:
> The re-segmentation added by the next patch splits an oversized TCP GSO skb
> into several GSO skbs which fit the device limits.  That needs the GSO
> engine to group several MSS segments into one output skb, so let callers
> set the number of MSS segments each output skb may carry and pass it
> through the existing __skb_gso_segment() entry point.  Ordinary callers
> pass zero for no limit.
> 
> Without max_segs, skb_segment() groups several MSS into one output skb only
> for a device which advertises NETIF_F_GSO_PARTIAL or for a skb with a
> frag_list which can be split into uniform pieces, and falls back to one
> segment per skb otherwise.  A caller which sets max_segs asks for the
> grouping regardless, so the block which makes that decision is skipped.
> Callers which pass zero keep it, and the re-segmentation path only runs for
> an unencapsulated TCP skb without a frag_list.
> 
> The output stays a GSO skb: gso_size is the original MSS and gso_segs is
> the number of MSS it holds, so a downstream device can still perform
> ordinary TSO.  Store max_segs in the existing skb_gso_cb scratch context,
> alongside the call-local data_offset and mac_offset fields, so that the
> segmentation methods keep their signature.  A zero max_segs value means
> that no limit is active; it is not a persistent skb flag.
> 
> Assisted-by: LLM

Reviewed-by: Willem de Bruijn <willemb@google.com>

If respinning, consider asking the LLM to rewrite commit messages to be
more concise.

> @@ -4839,7 +4840,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  	csum = !!can_checksum_protocol(features, proto);
>  
>  	if (sg && csum && !gso_by_frags)  {
> -		if (!(features & NETIF_F_GSO_PARTIAL)) {
> +		if (!max_segs && !(features & NETIF_F_GSO_PARTIAL)) {
>  			struct sk_buff *iter;
>  			unsigned int frag_len;
>  
> @@ -4874,7 +4875,10 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  		 * now.
>  		 */
>  		DEBUG_NET_WARN_ON_ONCE(len / mss > GSO_MAX_SEGS);
> -		partial_segs = min(len / mss, GSO_MAX_SEGS);
> +		if (max_segs)
> +			partial_segs = min(len / mss, max_segs);
> +		else
> +			partial_segs = min(len / mss, GSO_MAX_SEGS);
>  		if (partial_segs > 1)
>  			mss *= partial_segs;
>  		else

Not new with this series, nor high priority, so only if respinning:

Consider adding

@@ -5121,7 +5121,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
                }
 
                if (tail->len - doffset <= gso_size)
-                       skb_shinfo(tail)->gso_size = 0;
+                       skb_gso_reset(tail);
                else if (tail != segs)

Because packets looped into the Rx path will sometimes have their
gso_segs used without first checking gso_size /  skb_is_gso. For instance
in ip_rcv_core __IP_ADD_STATS.

Additionally for patch 5/5, consider one testcase that resegments, but for
which the last skb is not a GSO skb.

> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 21870341432552..e793aead68372d 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -375,7 +375,7 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb,
>  	int err;
>  
>  	BUILD_BUG_ON(sizeof(*OVS_CB(skb)) > SKB_GSO_CB_OFFSET);
> -	segs = __skb_gso_segment(skb, NETIF_F_SG, false);
> +	segs = __skb_gso_segment(skb, NETIF_F_SG, false, 0);
>  	if (IS_ERR(segs))
>  		return PTR_ERR(segs);
>  	if (segs == NULL)
> -- 
> 2.47.3
>