Re: [PATCH net-next v2 02/12] udp: gso: Simplify handling length in GSO_PARTIAL

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

Alice Mikityanska wrote:
> From: Alice Mikityanska <alice@isovalent.com>
> 
> Taking further the idea of commit b10b446ce7ad ("udp: gso: Use single
> MSS length in UDP header for GSO_PARTIAL"), simplify the implementation
> and fix the checksum (apparently ignored by hardware anyway).
> 
> The mentioned commit started using msslen for uh->len, but still uses
> newlen to adjust uh->check. If the formula for check is fixed, newlen is
> assigned but never used before the loop, and newlen is overwritten after
> the loop. This makes msslen not really necessary, as we can reuse
> newlen, if we don't adjust mss before. 

That's a big if. Why is the udp length now set to the segment length,
and not to the GSO packet length, as before?

> The adjustment of mss can be
> simply dropped, because mss is not used anywhere else below.
> 
> This brings us back to one variable, drops an unneeded arithmetic for
> mss, and fixes the UDP checksum.
> 
> Signed-off-by: Alice Mikityanska <alice@isovalent.com>
> Cc: Gal Pressman <gal@nvidia.com>
> ---
>  net/ipv4/udp_offload.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 6b1654c1ad4a..e831234326c4 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -483,11 +483,11 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  	struct sock *sk = gso_skb->sk;
>  	unsigned int sum_truesize = 0;
>  	struct sk_buff *segs, *seg;
> -	__be16 newlen, msslen;
>  	struct udphdr *uh;
>  	unsigned int mss;
>  	bool copy_dtor;
>  	__sum16 check;
> +	__be16 newlen;
>  	int ret = 0;
>  
>  	mss = skb_shinfo(gso_skb)->gso_size;
> @@ -556,15 +556,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  		return segs;
>  	}
>  
> -	msslen = htons(sizeof(*uh) + mss);
> -
> -	/* GSO partial and frag_list segmentation only requires splitting
> -	 * the frame into an MSS multiple and possibly a remainder, both
> -	 * cases return a GSO skb. So update the mss now.
> -	 */
> -	if (skb_is_gso(segs))
> -		mss *= skb_shinfo(segs)->gso_segs;
> -
>  	seg = segs;
>  	uh = udp_hdr(seg);
>  
> @@ -587,7 +578,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  		if (!seg->next)
>  			break;
>  
> -		uh->len = msslen;
> +		uh->len = newlen;
>  		uh->check = check;
>  
>  		if (seg->ip_summed == CHECKSUM_PARTIAL)
> -- 
> 2.52.0
>