diff mbox series

[net-next,2/5] udp: Do not pass checksum or MSS as parameters

Message ID 20180504003326.4496.54432.stgit@localhost.localdomain
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series UDP GSO Segmentation clean-ups | expand

Commit Message

Alexander H Duyck May 4, 2018, 12:33 a.m. UTC 
From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch is meant to be a start at cleaning up some of the UDP GSO
segmentation code. Specifically we were passing mss and a recomputed
checksum when we really didn't need to. The function itself could derive
that information based on the already provided checksum, the length of the
packet stored in the UDP header, and the gso_size.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 include/net/udp.h      |    3 +-
 net/ipv4/udp_offload.c |   61 +++++++++++++++++++++++++++++++-----------------
 net/ipv6/udp_offload.c |    7 +-----
 3 files changed, 41 insertions(+), 30 deletions(-)

Comments

Eric Dumazet May 4, 2018, 1:56 a.m. UTC | #1 
On 05/03/2018 05:33 PM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> This patch is meant to be a start at cleaning up some of the UDP GSO
> segmentation code. Specifically we were passing mss and a recomputed
> checksum when we really didn't need to. The function itself could derive
> that information based on the already provided checksum, the length of the
> packet stored in the UDP header, and the gso_size.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  include/net/udp.h      |    3 +-
>  net/ipv4/udp_offload.c |   61 +++++++++++++++++++++++++++++++-----------------
>  net/ipv6/udp_offload.c |    7 +-----
>  3 files changed, 41 insertions(+), 30 deletions(-)
>

There are really a lot of changes in this patch (not mentioned in changelog).

Can you please split all this in small patches ?
Alexander H Duyck May 4, 2018, 2:27 p.m. UTC | #2 
On Thu, May 3, 2018 at 6:56 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/03/2018 05:33 PM, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> This patch is meant to be a start at cleaning up some of the UDP GSO
>> segmentation code. Specifically we were passing mss and a recomputed
>> checksum when we really didn't need to. The function itself could derive
>> that information based on the already provided checksum, the length of the
>> packet stored in the UDP header, and the gso_size.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>>  include/net/udp.h      |    3 +-
>>  net/ipv4/udp_offload.c |   61 +++++++++++++++++++++++++++++++-----------------
>>  net/ipv6/udp_offload.c |    7 +-----
>>  3 files changed, 41 insertions(+), 30 deletions(-)
>>
>
> There are really a lot of changes in this patch (not mentioned in changelog).
>
> Can you please split all this in small patches ?

I'll see what I can do for the next version. I think I can come up
with an in-between step that should help to cut this down in size.

Thanks.

- Alex
diff mbox series

Patch

diff --git a/include/net/udp.h b/include/net/udp.h
index 05990746810e..9289b6425032 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -175,8 +175,7 @@  struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 int udp_gro_complete(struct sk_buff *skb, int nhoff, udp_lookup_t lookup);
 
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
-				  netdev_features_t features,
-				  unsigned int mss, __sum16 check);
+				  netdev_features_t features);
 
 static inline struct udphdr *udp_gro_udphdr(struct sk_buff *skb)
 {
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 006257092f06..946d06d2aa0c 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -188,19 +188,23 @@  struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 EXPORT_SYMBOL(skb_udp_tunnel_segment);
 
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
-				  netdev_features_t features,
-				  unsigned int mss, __sum16 check)
+				  netdev_features_t features)
 {
+	struct sk_buff *seg, *segs = ERR_PTR(-EINVAL);
 	struct sock *sk = gso_skb->sk;
 	unsigned int sum_truesize = 0;
-	struct sk_buff *segs, *seg;
-	unsigned int hdrlen;
 	struct udphdr *uh;
+	unsigned int mss;
+	__sum16 check;
+	__be16 newlen;
 
+	mss = skb_shinfo(gso_skb)->gso_size;
 	if (gso_skb->len <= sizeof(*uh) + mss)
-		return ERR_PTR(-EINVAL);
+		goto out;
+
+	if (!pskb_may_pull(gso_skb, sizeof(*uh)))
+		goto out;
 
-	hdrlen = gso_skb->data - skb_mac_header(gso_skb);
 	skb_pull(gso_skb, sizeof(*uh));
 
 	/* clear destructor to avoid skb_segment assigning it to tail */
@@ -213,24 +217,42 @@  struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 		return segs;
 	}
 
-	for (seg = segs; seg; seg = seg->next) {
-		uh = udp_hdr(seg);
-		uh->len = htons(seg->len - hdrlen);
-		uh->check = check;
+	seg = segs;
+	uh = udp_hdr(seg);
 
-		/* last packet can be partial gso_size */
-		if (!seg->next)
-			csum_replace2(&uh->check, htons(mss),
-				      htons(seg->len - hdrlen - sizeof(*uh)));
+	/* compute checksum adjustment based on old length versus new */
+	newlen = htons(sizeof(*uh) + mss);
+	check = ~csum_fold((__force __wsum)((__force u32)uh->check +
+					    ((__force u32)uh->len ^ 0xFFFF) +
+					    (__force u32)newlen));
 
-		uh->check = ~uh->check;
+	for (;;) {
 		seg->destructor = sock_wfree;
 		seg->sk = sk;
 		sum_truesize += seg->truesize;
+
+		if (!seg->next)
+			break;
+
+		uh->len = newlen;
+		uh->check = check;
+
+		seg = seg->next;
+		uh = udp_hdr(seg);
 	}
 
-	refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
+	/* last packet can be partial gso_size, account for that in checksum */
+	newlen = htons(skb_tail_pointer(seg) - skb_transport_header(seg) +
+		       seg->data_len);
+	check = ~csum_fold((__force __wsum)((__force u32)uh->check +
+					    ((__force u32)uh->len ^ 0xFFFF)  +
+					    (__force u32)newlen));
+	uh->len = newlen;
+	uh->check = check;
 
+	/* update refcount for the packet */
+	refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
+out:
 	return segs;
 }
 EXPORT_SYMBOL_GPL(__udp_gso_segment);
@@ -238,15 +260,10 @@  struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 static struct sk_buff *__udp4_gso_segment(struct sk_buff *gso_skb,
 					  netdev_features_t features)
 {
-	const struct iphdr *iph = ip_hdr(gso_skb);
-	unsigned int mss = skb_shinfo(gso_skb)->gso_size;
-
 	if (!can_checksum_protocol(features, htons(ETH_P_IP)))
 		return ERR_PTR(-EIO);
 
-	return __udp_gso_segment(gso_skb, features, mss,
-				 udp_v4_check(sizeof(struct udphdr) + mss,
-					      iph->saddr, iph->daddr, 0));
+	return __udp_gso_segment(gso_skb, features);
 }
 
 static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index f7b85b1e6b3e..61e34f1d2fa2 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -20,15 +20,10 @@ 
 static struct sk_buff *__udp6_gso_segment(struct sk_buff *gso_skb,
 					  netdev_features_t features)
 {
-	const struct ipv6hdr *ip6h = ipv6_hdr(gso_skb);
-	unsigned int mss = skb_shinfo(gso_skb)->gso_size;
-
 	if (!can_checksum_protocol(features, htons(ETH_P_IPV6)))
 		return ERR_PTR(-EIO);
 
-	return __udp_gso_segment(gso_skb, features, mss,
-				 udp_v6_check(sizeof(struct udphdr) + mss,
-					      &ip6h->saddr, &ip6h->daddr, 0));
+	return __udp_gso_segment(gso_skb, features);
 }
 
 static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,