netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] net: Eliminate gso_send_check
@ 2014-09-20 21:52 Tom Herbert
  2014-09-20 21:52 ` [PATCH net-next 1/3] tcp: move logic out of tcp_v[64]_gso_send_check Tom Herbert
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Tom Herbert @ 2014-09-20 21:52 UTC (permalink / raw)
  To: davem, netdev

gso_send_check presents a lot of complexity for what it is being used
for. It seems that there are only two cases where it might be effective:
TCP and UFO paths. In these cases, the gso_send_check function
initializes the TCP or UDP checksum respectively to the pseudo header
checksum so that the checksum computation is appropriately offloaded or
computed in the gso_segment functions. The gso_send_check functions
are only called from dev.c in skb_mac_gso_segment when ip_summed !=
CHECKSUM_PARTIAL (which seems very unlikely in TCP case). We can move
the logic of this into the respective gso_segment functions where the
checksum is initialized if ip_summed != CHECKSUM_PARTIAL.

With the above cases handled, gso_send_check is no longer needed, so
we can remove all uses of it and the fields in the offload callbacks.
With this change, ip_summed in the skb should be preserved though all
the layers of gso_segment calls.

In follow-on patches, we may be able to remove the check setup code in
tcp_gso_segment if we can guarantee that ip_summed will always be
CHECKSUM_PARTIAL (verify all paths and probably add an assert in
tcp_gro_segment).

Tested these patches by:
  - netperf TCP_STREAM test with GSO enabled
  - Forced ip_summed != CHECKSUM_PARTIAL with above
  - Ran UDP_RR with 10000 request size over GRE tunnel. This exercised
    UFO path.

Tom Herbert (3):
  tcp: move logic out of tcp_v[64]_gso_send_check
  udp: move logic out of udp[46]_ufo_send_check
  net: Remove gso_send_check as an offload callback

 include/linux/netdevice.h |  1 -
 net/core/dev.c            | 10 ----------
 net/ipv4/af_inet.c        | 36 ------------------------------------
 net/ipv4/gre_offload.c    | 11 +++--------
 net/ipv4/tcp_offload.c    | 45 +++++++++++++++++++++++----------------------
 net/ipv4/udp_offload.c    | 43 +++++++++++++++----------------------------
 net/ipv6/ip6_offload.c    | 27 ---------------------------
 net/ipv6/tcpv6_offload.c  | 43 ++++++++++++++++++++++++-------------------
 net/ipv6/udp_offload.c    | 46 ++++++++++++++++++----------------------------
 net/mpls/mpls_gso.c       |  7 -------
 10 files changed, 83 insertions(+), 186 deletions(-)

-- 
2.1.0.rc2.206.gedb03e5

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH net-next 1/3] tcp: move logic out of tcp_v[64]_gso_send_check
  2014-09-20 21:52 [PATCH net-next 0/3] net: Eliminate gso_send_check Tom Herbert
@ 2014-09-20 21:52 ` Tom Herbert
  2014-09-21  1:04   ` Eric Dumazet
  2014-09-20 21:52 ` [PATCH net-next 2/3] udp: move logic out of udp[46]_ufo_send_check Tom Herbert
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Tom Herbert @ 2014-09-20 21:52 UTC (permalink / raw)
  To: davem, netdev

In tcp_v[46]_gso_send_check the TCP checksum is initialized to the
pseudo header checksum using __tcp_v[46]_send_check. We can move this
logic into new tcp[46]_gso_segment functions to be done when
ip_summed != CHECKSUM_PARTIAL (ip_summed == CHECKSUM_PARTIAL should be
the common case, possibly always true when taking GSO path). After this
change tcp_v[46]_gso_send_check is no-op.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 net/ipv4/tcp_offload.c   | 39 +++++++++++++++++++++++----------------
 net/ipv6/tcpv6_offload.c | 37 ++++++++++++++++++++++++-------------
 2 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 7291253..7cd12b0 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -29,6 +29,28 @@ static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq,
 	}
 }
 
+struct sk_buff *tcp4_gso_segment(struct sk_buff *skb,
+				 netdev_features_t features)
+{
+	if (!pskb_may_pull(skb, sizeof(struct tcphdr)))
+		return ERR_PTR(-EINVAL);
+
+	if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
+		const struct iphdr *iph = ip_hdr(skb);
+		struct tcphdr *th = tcp_hdr(skb);
+
+		/* Set up checksum pseudo header, usually expect stack to
+		 * have done this already.
+		 */
+
+		th->check = 0;
+		skb->ip_summed = CHECKSUM_PARTIAL;
+		__tcp_v4_send_check(skb, iph->saddr, iph->daddr);
+	}
+
+	return tcp_gso_segment(skb, features);
+}
+
 struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
 				netdev_features_t features)
 {
@@ -44,9 +66,6 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
 	__sum16 newcheck;
 	bool ooo_okay, copy_destructor;
 
-	if (!pskb_may_pull(skb, sizeof(*th)))
-		goto out;
-
 	th = tcp_hdr(skb);
 	thlen = th->doff * 4;
 	if (thlen < sizeof(*th))
@@ -271,18 +290,6 @@ EXPORT_SYMBOL(tcp_gro_complete);
 
 static int tcp_v4_gso_send_check(struct sk_buff *skb)
 {
-	const struct iphdr *iph;
-	struct tcphdr *th;
-
-	if (!pskb_may_pull(skb, sizeof(*th)))
-		return -EINVAL;
-
-	iph = ip_hdr(skb);
-	th = tcp_hdr(skb);
-
-	th->check = 0;
-	skb->ip_summed = CHECKSUM_PARTIAL;
-	__tcp_v4_send_check(skb, iph->saddr, iph->daddr);
 	return 0;
 }
 
@@ -314,7 +321,7 @@ static int tcp4_gro_complete(struct sk_buff *skb, int thoff)
 static const struct net_offload tcpv4_offload = {
 	.callbacks = {
 		.gso_send_check	=	tcp_v4_gso_send_check,
-		.gso_segment	=	tcp_gso_segment,
+		.gso_segment	=	tcp4_gso_segment,
 		.gro_receive	=	tcp4_gro_receive,
 		.gro_complete	=	tcp4_gro_complete,
 	},
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index dbb3d92..9625315 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -17,18 +17,6 @@
 
 static int tcp_v6_gso_send_check(struct sk_buff *skb)
 {
-	const struct ipv6hdr *ipv6h;
-	struct tcphdr *th;
-
-	if (!pskb_may_pull(skb, sizeof(*th)))
-		return -EINVAL;
-
-	ipv6h = ipv6_hdr(skb);
-	th = tcp_hdr(skb);
-
-	th->check = 0;
-	skb->ip_summed = CHECKSUM_PARTIAL;
-	__tcp_v6_send_check(skb, &ipv6h->saddr, &ipv6h->daddr);
 	return 0;
 }
 
@@ -58,10 +46,33 @@ static int tcp6_gro_complete(struct sk_buff *skb, int thoff)
 	return tcp_gro_complete(skb);
 }
 
+struct sk_buff *tcp6_gso_segment(struct sk_buff *skb,
+				 netdev_features_t features)
+{
+	struct tcphdr *th;
+
+	if (!pskb_may_pull(skb, sizeof(*th)))
+		return ERR_PTR(-EINVAL);
+
+	if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
+		const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
+		struct tcphdr *th = tcp_hdr(skb);
+
+		/* Set up pseudo header, usually expect stack to have done
+		 * this.
+		 */
+
+		th->check = 0;
+		skb->ip_summed = CHECKSUM_PARTIAL;
+		__tcp_v6_send_check(skb, &ipv6h->saddr, &ipv6h->daddr);
+	}
+
+	return tcp_gso_segment(skb, features);
+}
 static const struct net_offload tcpv6_offload = {
 	.callbacks = {
 		.gso_send_check	=	tcp_v6_gso_send_check,
-		.gso_segment	=	tcp_gso_segment,
+		.gso_segment	=	tcp6_gso_segment,
 		.gro_receive	=	tcp6_gro_receive,
 		.gro_complete	=	tcp6_gro_complete,
 	},
-- 
2.1.0.rc2.206.gedb03e5

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH net-next 2/3] udp: move logic out of udp[46]_ufo_send_check
  2014-09-20 21:52 [PATCH net-next 0/3] net: Eliminate gso_send_check Tom Herbert
  2014-09-20 21:52 ` [PATCH net-next 1/3] tcp: move logic out of tcp_v[64]_gso_send_check Tom Herbert
@ 2014-09-20 21:52 ` Tom Herbert
  2014-09-21  1:12   ` Eric Dumazet
  2014-09-20 21:52 ` [PATCH net-next 3/3] net: Remove gso_send_check as an offload callback Tom Herbert
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Tom Herbert @ 2014-09-20 21:52 UTC (permalink / raw)
  To: davem, netdev

In udp[46]_ufo_send_check the UDP checksum initialized to the pseudo
header checksum. We can move this logic into udp[46]_ufo_fragment.
After this change udp[64]_ufo_send_check is a no-op.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 net/ipv4/udp_offload.c | 37 +++++++++++++++----------------------
 net/ipv6/udp_offload.c | 40 ++++++++++++++++++----------------------
 2 files changed, 33 insertions(+), 44 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index d7c43f7..2918cc9 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -27,23 +27,6 @@ struct udp_offload_priv {
 
 static int udp4_ufo_send_check(struct sk_buff *skb)
 {
-	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
-		return -EINVAL;
-
-	if (likely(!skb->encapsulation)) {
-		const struct iphdr *iph;
-		struct udphdr *uh;
-
-		iph = ip_hdr(skb);
-		uh = udp_hdr(skb);
-
-		uh->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, skb->len,
-				IPPROTO_UDP, 0);
-		skb->csum_start = skb_transport_header(skb) - skb->head;
-		skb->csum_offset = offsetof(struct udphdr, check);
-		skb->ip_summed = CHECKSUM_PARTIAL;
-	}
-
 	return 0;
 }
 
@@ -128,8 +111,9 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 {
 	struct sk_buff *segs = ERR_PTR(-EINVAL);
 	unsigned int mss;
-	int offset;
 	__wsum csum;
+	struct udphdr *uh;
+	struct iphdr *iph;
 
 	if (skb->encapsulation &&
 	    (skb_shinfo(skb)->gso_type &
@@ -138,6 +122,9 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 		goto out;
 	}
 
+	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
+		goto out;
+
 	mss = skb_shinfo(skb)->gso_size;
 	if (unlikely(skb->len <= mss))
 		goto out;
@@ -165,10 +152,16 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 	 * HW cannot do checksum of UDP packets sent as multiple
 	 * IP fragments.
 	 */
-	offset = skb_checksum_start_offset(skb);
-	csum = skb_checksum(skb, offset, skb->len - offset, 0);
-	offset += skb->csum_offset;
-	*(__sum16 *)(skb->data + offset) = csum_fold(csum);
+
+	uh = udp_hdr(skb);
+	iph = ip_hdr(skb);
+
+	uh->check = 0;
+	csum = skb_checksum(skb, 0, skb->len, 0);
+	uh->check = udp_v4_check(skb->len, iph->saddr, iph->daddr, csum);
+	if (uh->check == 0)
+		uh->check = CSUM_MANGLED_0;
+
 	skb->ip_summed = CHECKSUM_NONE;
 
 	/* Fragment the skb. IP headers of the fragments are updated in
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index de85f80..e4af643 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -19,23 +19,6 @@
 
 static int udp6_ufo_send_check(struct sk_buff *skb)
 {
-	const struct ipv6hdr *ipv6h;
-	struct udphdr *uh;
-
-	if (!pskb_may_pull(skb, sizeof(*uh)))
-		return -EINVAL;
-
-	if (likely(!skb->encapsulation)) {
-		ipv6h = ipv6_hdr(skb);
-		uh = udp_hdr(skb);
-
-		uh->check = ~csum_ipv6_magic(&ipv6h->saddr, &ipv6h->daddr, skb->len,
-					     IPPROTO_UDP, 0);
-		skb->csum_start = skb_transport_header(skb) - skb->head;
-		skb->csum_offset = offsetof(struct udphdr, check);
-		skb->ip_summed = CHECKSUM_PARTIAL;
-	}
-
 	return 0;
 }
 
@@ -49,7 +32,6 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 	u8 *packet_start, *prevhdr;
 	u8 nexthdr;
 	u8 frag_hdr_sz = sizeof(struct frag_hdr);
-	int offset;
 	__wsum csum;
 	int tnl_hlen;
 
@@ -83,13 +65,27 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 	    (SKB_GSO_UDP_TUNNEL|SKB_GSO_UDP_TUNNEL_CSUM))
 		segs = skb_udp_tunnel_segment(skb, features);
 	else {
+		const struct ipv6hdr *ipv6h;
+		struct udphdr *uh;
+
+		if (!pskb_may_pull(skb, sizeof(struct udphdr)))
+			goto out;
+
 		/* Do software UFO. Complete and fill in the UDP checksum as HW cannot
 		 * do checksum of UDP packets sent as multiple IP fragments.
 		 */
-		offset = skb_checksum_start_offset(skb);
-		csum = skb_checksum(skb, offset, skb->len - offset, 0);
-		offset += skb->csum_offset;
-		*(__sum16 *)(skb->data + offset) = csum_fold(csum);
+
+		uh = udp_hdr(skb);
+		ipv6h = ipv6_hdr(skb);
+
+		uh->check = 0;
+		csum = skb_checksum(skb, 0, skb->len, 0);
+		uh->check = udp_v6_check(skb->len, &ipv6h->saddr,
+					  &ipv6h->daddr, csum);
+
+		if (uh->check == 0)
+			uh->check = CSUM_MANGLED_0;
+
 		skb->ip_summed = CHECKSUM_NONE;
 
 		/* Check if there is enough headroom to insert fragment header. */
-- 
2.1.0.rc2.206.gedb03e5

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH net-next 3/3] net: Remove gso_send_check as an offload callback
  2014-09-20 21:52 [PATCH net-next 0/3] net: Eliminate gso_send_check Tom Herbert
  2014-09-20 21:52 ` [PATCH net-next 1/3] tcp: move logic out of tcp_v[64]_gso_send_check Tom Herbert
  2014-09-20 21:52 ` [PATCH net-next 2/3] udp: move logic out of udp[46]_ufo_send_check Tom Herbert
@ 2014-09-20 21:52 ` Tom Herbert
  2014-09-21  1:04 ` [PATCH net-next 0/3] net: Eliminate gso_send_check Eric Dumazet
  2014-09-26  4:23 ` David Miller
  4 siblings, 0 replies; 9+ messages in thread
From: Tom Herbert @ 2014-09-20 21:52 UTC (permalink / raw)
  To: davem, netdev

The send_check logic was only interesting in cases of TCP offload and
UDP UFO where the checksum needed to be initialized to the pseudo
header checksum. Now we've moved that logic into the related
gso_segment functions so gso_send_check is no longer needed.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 include/linux/netdevice.h |  1 -
 net/core/dev.c            | 10 ----------
 net/ipv4/af_inet.c        | 36 ------------------------------------
 net/ipv4/gre_offload.c    | 11 +++--------
 net/ipv4/tcp_offload.c    |  6 ------
 net/ipv4/udp_offload.c    |  6 ------
 net/ipv6/ip6_offload.c    | 27 ---------------------------
 net/ipv6/tcpv6_offload.c  |  6 ------
 net/ipv6/udp_offload.c    |  6 ------
 net/mpls/mpls_gso.c       |  7 -------
 10 files changed, 3 insertions(+), 113 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4354b43..9f5d293 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1911,7 +1911,6 @@ struct packet_type {
 struct offload_callbacks {
 	struct sk_buff		*(*gso_segment)(struct sk_buff *skb,
 						netdev_features_t features);
-	int			(*gso_send_check)(struct sk_buff *skb);
 	struct sk_buff		**(*gro_receive)(struct sk_buff **head,
 					       struct sk_buff *skb);
 	int			(*gro_complete)(struct sk_buff *skb, int nhoff);
diff --git a/net/core/dev.c b/net/core/dev.c
index e916ba8..734bca4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2422,16 +2422,6 @@ struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,
 	rcu_read_lock();
 	list_for_each_entry_rcu(ptype, &offload_base, list) {
 		if (ptype->type == type && ptype->callbacks.gso_segment) {
-			if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
-				int err;
-
-				err = ptype->callbacks.gso_send_check(skb);
-				segs = ERR_PTR(err);
-				if (err || skb_gso_ok(skb, features))
-					break;
-				__skb_push(skb, (skb->data -
-						 skb_network_header(skb)));
-			}
 			segs = ptype->callbacks.gso_segment(skb, features);
 			break;
 		}
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 72011cc..28e589c 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1197,40 +1197,6 @@ int inet_sk_rebuild_header(struct sock *sk)
 }
 EXPORT_SYMBOL(inet_sk_rebuild_header);
 
-static int inet_gso_send_check(struct sk_buff *skb)
-{
-	const struct net_offload *ops;
-	const struct iphdr *iph;
-	int proto;
-	int ihl;
-	int err = -EINVAL;
-
-	if (unlikely(!pskb_may_pull(skb, sizeof(*iph))))
-		goto out;
-
-	iph = ip_hdr(skb);
-	ihl = iph->ihl * 4;
-	if (ihl < sizeof(*iph))
-		goto out;
-
-	proto = iph->protocol;
-
-	/* Warning: after this point, iph might be no longer valid */
-	if (unlikely(!pskb_may_pull(skb, ihl)))
-		goto out;
-	__skb_pull(skb, ihl);
-
-	skb_reset_transport_header(skb);
-	err = -EPROTONOSUPPORT;
-
-	ops = rcu_dereference(inet_offloads[proto]);
-	if (likely(ops && ops->callbacks.gso_send_check))
-		err = ops->callbacks.gso_send_check(skb);
-
-out:
-	return err;
-}
-
 static struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 					netdev_features_t features)
 {
@@ -1655,7 +1621,6 @@ static int ipv4_proc_init(void);
 static struct packet_offload ip_packet_offload __read_mostly = {
 	.type = cpu_to_be16(ETH_P_IP),
 	.callbacks = {
-		.gso_send_check = inet_gso_send_check,
 		.gso_segment = inet_gso_segment,
 		.gro_receive = inet_gro_receive,
 		.gro_complete = inet_gro_complete,
@@ -1664,7 +1629,6 @@ static struct packet_offload ip_packet_offload __read_mostly = {
 
 static const struct net_offload ipip_offload = {
 	.callbacks = {
-		.gso_send_check = inet_gso_send_check,
 		.gso_segment	= inet_gso_segment,
 		.gro_receive	= inet_gro_receive,
 		.gro_complete	= inet_gro_complete,
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index d3fe2ac..a777295 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -15,13 +15,6 @@
 #include <net/protocol.h>
 #include <net/gre.h>
 
-static int gre_gso_send_check(struct sk_buff *skb)
-{
-	if (!skb->encapsulation)
-		return -EINVAL;
-	return 0;
-}
-
 static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 				       netdev_features_t features)
 {
@@ -46,6 +39,9 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 				  SKB_GSO_IPIP)))
 		goto out;
 
+	if (!skb->encapsulation)
+		goto out;
+
 	if (unlikely(!pskb_may_pull(skb, sizeof(*greh))))
 		goto out;
 
@@ -256,7 +252,6 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
 
 static const struct net_offload gre_offload = {
 	.callbacks = {
-		.gso_send_check = gre_gso_send_check,
 		.gso_segment = gre_gso_segment,
 		.gro_receive = gre_gro_receive,
 		.gro_complete = gre_gro_complete,
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 7cd12b0..5b90f2f 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -288,11 +288,6 @@ int tcp_gro_complete(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(tcp_gro_complete);
 
-static int tcp_v4_gso_send_check(struct sk_buff *skb)
-{
-	return 0;
-}
-
 static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *skb)
 {
 	/* Don't bother verifying checksum if we're going to flush anyway. */
@@ -320,7 +315,6 @@ static int tcp4_gro_complete(struct sk_buff *skb, int thoff)
 
 static const struct net_offload tcpv4_offload = {
 	.callbacks = {
-		.gso_send_check	=	tcp_v4_gso_send_check,
 		.gso_segment	=	tcp4_gso_segment,
 		.gro_receive	=	tcp4_gro_receive,
 		.gro_complete	=	tcp4_gro_complete,
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 2918cc9..19ebe6a 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -25,11 +25,6 @@ struct udp_offload_priv {
 	struct udp_offload_priv __rcu *next;
 };
 
-static int udp4_ufo_send_check(struct sk_buff *skb)
-{
-	return 0;
-}
-
 struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 				       netdev_features_t features)
 {
@@ -346,7 +341,6 @@ static int udp4_gro_complete(struct sk_buff *skb, int nhoff)
 
 static const struct net_offload udpv4_offload = {
 	.callbacks = {
-		.gso_send_check = udp4_ufo_send_check,
 		.gso_segment = udp4_ufo_fragment,
 		.gro_receive  =	udp4_gro_receive,
 		.gro_complete =	udp4_gro_complete,
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 9952f3f..9034f76 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -53,31 +53,6 @@ static int ipv6_gso_pull_exthdrs(struct sk_buff *skb, int proto)
 	return proto;
 }
 
-static int ipv6_gso_send_check(struct sk_buff *skb)
-{
-	const struct ipv6hdr *ipv6h;
-	const struct net_offload *ops;
-	int err = -EINVAL;
-
-	if (unlikely(!pskb_may_pull(skb, sizeof(*ipv6h))))
-		goto out;
-
-	ipv6h = ipv6_hdr(skb);
-	__skb_pull(skb, sizeof(*ipv6h));
-	err = -EPROTONOSUPPORT;
-
-	ops = rcu_dereference(inet6_offloads[
-		ipv6_gso_pull_exthdrs(skb, ipv6h->nexthdr)]);
-
-	if (likely(ops && ops->callbacks.gso_send_check)) {
-		skb_reset_transport_header(skb);
-		err = ops->callbacks.gso_send_check(skb);
-	}
-
-out:
-	return err;
-}
-
 static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
 	netdev_features_t features)
 {
@@ -306,7 +281,6 @@ out_unlock:
 static struct packet_offload ipv6_packet_offload __read_mostly = {
 	.type = cpu_to_be16(ETH_P_IPV6),
 	.callbacks = {
-		.gso_send_check = ipv6_gso_send_check,
 		.gso_segment = ipv6_gso_segment,
 		.gro_receive = ipv6_gro_receive,
 		.gro_complete = ipv6_gro_complete,
@@ -315,7 +289,6 @@ static struct packet_offload ipv6_packet_offload __read_mostly = {
 
 static const struct net_offload sit_offload = {
 	.callbacks = {
-		.gso_send_check = ipv6_gso_send_check,
 		.gso_segment	= ipv6_gso_segment,
 		.gro_receive	= ipv6_gro_receive,
 		.gro_complete	= ipv6_gro_complete,
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index 9625315..c1ab771 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -15,11 +15,6 @@
 #include <net/ip6_checksum.h>
 #include "ip6_offload.h"
 
-static int tcp_v6_gso_send_check(struct sk_buff *skb)
-{
-	return 0;
-}
-
 static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 					 struct sk_buff *skb)
 {
@@ -71,7 +66,6 @@ struct sk_buff *tcp6_gso_segment(struct sk_buff *skb,
 }
 static const struct net_offload tcpv6_offload = {
 	.callbacks = {
-		.gso_send_check	=	tcp_v6_gso_send_check,
 		.gso_segment	=	tcp6_gso_segment,
 		.gro_receive	=	tcp6_gro_receive,
 		.gro_complete	=	tcp6_gro_complete,
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index e4af643..212ebfc 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -17,11 +17,6 @@
 #include <net/ip6_checksum.h>
 #include "ip6_offload.h"
 
-static int udp6_ufo_send_check(struct sk_buff *skb)
-{
-	return 0;
-}
-
 static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 					 netdev_features_t features)
 {
@@ -166,7 +161,6 @@ static int udp6_gro_complete(struct sk_buff *skb, int nhoff)
 
 static const struct net_offload udpv6_offload = {
 	.callbacks = {
-		.gso_send_check =	udp6_ufo_send_check,
 		.gso_segment	=	udp6_ufo_fragment,
 		.gro_receive	=	udp6_gro_receive,
 		.gro_complete	=	udp6_gro_complete,
diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
index 6b38d08..e28ed2e 100644
--- a/net/mpls/mpls_gso.c
+++ b/net/mpls/mpls_gso.c
@@ -65,15 +65,9 @@ out:
 	return segs;
 }
 
-static int mpls_gso_send_check(struct sk_buff *skb)
-{
-	return 0;
-}
-
 static struct packet_offload mpls_mc_offload = {
 	.type = cpu_to_be16(ETH_P_MPLS_MC),
 	.callbacks = {
-		.gso_send_check =	mpls_gso_send_check,
 		.gso_segment    =	mpls_gso_segment,
 	},
 };
@@ -81,7 +75,6 @@ static struct packet_offload mpls_mc_offload = {
 static struct packet_offload mpls_uc_offload = {
 	.type = cpu_to_be16(ETH_P_MPLS_UC),
 	.callbacks = {
-		.gso_send_check =	mpls_gso_send_check,
 		.gso_segment    =	mpls_gso_segment,
 	},
 };
-- 
2.1.0.rc2.206.gedb03e5

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/3] net: Eliminate gso_send_check
  2014-09-20 21:52 [PATCH net-next 0/3] net: Eliminate gso_send_check Tom Herbert
                   ` (2 preceding siblings ...)
  2014-09-20 21:52 ` [PATCH net-next 3/3] net: Remove gso_send_check as an offload callback Tom Herbert
@ 2014-09-21  1:04 ` Eric Dumazet
  2014-09-26  4:23 ` David Miller
  4 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2014-09-21  1:04 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, netdev

On Sat, 2014-09-20 at 14:52 -0700, Tom Herbert wrote:
> gso_send_check presents a lot of complexity for what it is being used
> for. It seems that there are only two cases where it might be effective:
> TCP and UFO paths. In these cases, the gso_send_check function
> initializes the TCP or UDP checksum respectively to the pseudo header
> checksum so that the checksum computation is appropriately offloaded or
> computed in the gso_segment functions. The gso_send_check functions
> are only called from dev.c in skb_mac_gso_segment when ip_summed !=
> CHECKSUM_PARTIAL (which seems very unlikely in TCP case). We can move
> the logic of this into the respective gso_segment functions where the
> checksum is initialized if ip_summed != CHECKSUM_PARTIAL.
> 
> With the above cases handled, gso_send_check is no longer needed, so
> we can remove all uses of it and the fields in the offload callbacks.
> With this change, ip_summed in the skb should be preserved though all
> the layers of gso_segment calls.
> 
> In follow-on patches, we may be able to remove the check setup code in
> tcp_gso_segment if we can guarantee that ip_summed will always be
> CHECKSUM_PARTIAL (verify all paths and probably add an assert in
> tcp_gro_segment).
> 
> Tested these patches by:
>   - netperf TCP_STREAM test with GSO enabled
>   - Forced ip_summed != CHECKSUM_PARTIAL with above
>   - Ran UDP_RR with 10000 request size over GRE tunnel. This exercised
>     UFO path.
> 
> Tom Herbert (3):
>   tcp: move logic out of tcp_v[64]_gso_send_check
>   udp: move logic out of udp[46]_ufo_send_check
>   net: Remove gso_send_check as an offload callback

Very very nice !

Acked-by: Eric Dumazet <edumazet@google.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] tcp: move logic out of tcp_v[64]_gso_send_check
  2014-09-20 21:52 ` [PATCH net-next 1/3] tcp: move logic out of tcp_v[64]_gso_send_check Tom Herbert
@ 2014-09-21  1:04   ` Eric Dumazet
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2014-09-21  1:04 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, netdev

On Sat, 2014-09-20 at 14:52 -0700, Tom Herbert wrote:
> In tcp_v[46]_gso_send_check the TCP checksum is initialized to the
> pseudo header checksum using __tcp_v[46]_send_check. We can move this
> logic into new tcp[46]_gso_segment functions to be done when
> ip_summed != CHECKSUM_PARTIAL (ip_summed == CHECKSUM_PARTIAL should be
> the common case, possibly always true when taking GSO path). After this
> change tcp_v[46]_gso_send_check is no-op.
> 
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
>  net/ipv4/tcp_offload.c   | 39 +++++++++++++++++++++++----------------
>  net/ipv6/tcpv6_offload.c | 37 ++++++++++++++++++++++++-------------
>  2 files changed, 47 insertions(+), 29 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] udp: move logic out of udp[46]_ufo_send_check
  2014-09-20 21:52 ` [PATCH net-next 2/3] udp: move logic out of udp[46]_ufo_send_check Tom Herbert
@ 2014-09-21  1:12   ` Eric Dumazet
  2014-09-21  1:29     ` Tom Herbert
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2014-09-21  1:12 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, netdev

On Sat, 2014-09-20 at 14:52 -0700, Tom Herbert wrote:
> In udp[46]_ufo_send_check the UDP checksum initialized to the pseudo
> header checksum. We can move this logic into udp[46]_ufo_fragment.
> After this change udp[64]_ufo_send_check is a no-op.
> 
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
>  net/ipv4/udp_offload.c | 37 +++++++++++++++----------------------
>  net/ipv6/udp_offload.c | 40 ++++++++++++++++++----------------------
>  2 files changed, 33 insertions(+), 44 deletions(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index d7c43f7..2918cc9 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -27,23 +27,6 @@ struct udp_offload_priv {
>  
>  static int udp4_ufo_send_check(struct sk_buff *skb)
>  {
> -	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> -		return -EINVAL;
> -
> -	if (likely(!skb->encapsulation)) {
> -		const struct iphdr *iph;
> -		struct udphdr *uh;
> -
> -		iph = ip_hdr(skb);
> -		uh = udp_hdr(skb);
> -
> -		uh->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, skb->len,
> -				IPPROTO_UDP, 0);
> -		skb->csum_start = skb_transport_header(skb) - skb->head;
> -		skb->csum_offset = offsetof(struct udphdr, check);
> -		skb->ip_summed = CHECKSUM_PARTIAL;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -128,8 +111,9 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
>  {
>  	struct sk_buff *segs = ERR_PTR(-EINVAL);
>  	unsigned int mss;
> -	int offset;
>  	__wsum csum;
> +	struct udphdr *uh;
> +	struct iphdr *iph;
>  
>  	if (skb->encapsulation &&
>  	    (skb_shinfo(skb)->gso_type &
> @@ -138,6 +122,9 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
>  		goto out;
>  	}
>  
> +	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> +		goto out;
> +
>  	mss = skb_shinfo(skb)->gso_size;
>  	if (unlikely(skb->len <= mss))
>  		goto out;
> @@ -165,10 +152,16 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
>  	 * HW cannot do checksum of UDP packets sent as multiple
>  	 * IP fragments.
>  	 */
> -	offset = skb_checksum_start_offset(skb);
> -	csum = skb_checksum(skb, offset, skb->len - offset, 0);
> -	offset += skb->csum_offset;
> -	*(__sum16 *)(skb->data + offset) = csum_fold(csum);
> +
> +	uh = udp_hdr(skb);
> +	iph = ip_hdr(skb);
> +
> +	uh->check = 0;
> +	csum = skb_checksum(skb, 0, skb->len, 0);
> +	uh->check = udp_v4_check(skb->len, iph->saddr, iph->daddr, csum);
> +	if (uh->check == 0)
> +		uh->check = CSUM_MANGLED_0;
> +
>  	skb->ip_summed = CHECKSUM_NONE;


Shouldn't it be changed to CHECKSUM_PARTIAL or something ?

udp4_ufo_send_check() was able to fill CHECKSUM_PARTIAL,
I fear I do not understand enough this part.

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] udp: move logic out of udp[46]_ufo_send_check
  2014-09-21  1:12   ` Eric Dumazet
@ 2014-09-21  1:29     ` Tom Herbert
  0 siblings, 0 replies; 9+ messages in thread
From: Tom Herbert @ 2014-09-21  1:29 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, Linux Netdev List

On Sat, Sep 20, 2014 at 6:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sat, 2014-09-20 at 14:52 -0700, Tom Herbert wrote:
>> In udp[46]_ufo_send_check the UDP checksum initialized to the pseudo
>> header checksum. We can move this logic into udp[46]_ufo_fragment.
>> After this change udp[64]_ufo_send_check is a no-op.
>>
>> Signed-off-by: Tom Herbert <therbert@google.com>
>> ---
>>  net/ipv4/udp_offload.c | 37 +++++++++++++++----------------------
>>  net/ipv6/udp_offload.c | 40 ++++++++++++++++++----------------------
>>  2 files changed, 33 insertions(+), 44 deletions(-)
>>
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index d7c43f7..2918cc9 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -27,23 +27,6 @@ struct udp_offload_priv {
>>
>>  static int udp4_ufo_send_check(struct sk_buff *skb)
>>  {
>> -     if (!pskb_may_pull(skb, sizeof(struct udphdr)))
>> -             return -EINVAL;
>> -
>> -     if (likely(!skb->encapsulation)) {
>> -             const struct iphdr *iph;
>> -             struct udphdr *uh;
>> -
>> -             iph = ip_hdr(skb);
>> -             uh = udp_hdr(skb);
>> -
>> -             uh->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, skb->len,
>> -                             IPPROTO_UDP, 0);
>> -             skb->csum_start = skb_transport_header(skb) - skb->head;
>> -             skb->csum_offset = offsetof(struct udphdr, check);
>> -             skb->ip_summed = CHECKSUM_PARTIAL;
>> -     }
>> -
>>       return 0;
>>  }
>>
>> @@ -128,8 +111,9 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
>>  {
>>       struct sk_buff *segs = ERR_PTR(-EINVAL);
>>       unsigned int mss;
>> -     int offset;
>>       __wsum csum;
>> +     struct udphdr *uh;
>> +     struct iphdr *iph;
>>
>>       if (skb->encapsulation &&
>>           (skb_shinfo(skb)->gso_type &
>> @@ -138,6 +122,9 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
>>               goto out;
>>       }
>>
>> +     if (!pskb_may_pull(skb, sizeof(struct udphdr)))
>> +             goto out;
>> +
>>       mss = skb_shinfo(skb)->gso_size;
>>       if (unlikely(skb->len <= mss))
>>               goto out;
>> @@ -165,10 +152,16 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
>>        * HW cannot do checksum of UDP packets sent as multiple
>>        * IP fragments.
>>        */
>> -     offset = skb_checksum_start_offset(skb);
>> -     csum = skb_checksum(skb, offset, skb->len - offset, 0);
>> -     offset += skb->csum_offset;
>> -     *(__sum16 *)(skb->data + offset) = csum_fold(csum);
>> +
>> +     uh = udp_hdr(skb);
>> +     iph = ip_hdr(skb);
>> +
>> +     uh->check = 0;
>> +     csum = skb_checksum(skb, 0, skb->len, 0);
>> +     uh->check = udp_v4_check(skb->len, iph->saddr, iph->daddr, csum);
>> +     if (uh->check == 0)
>> +             uh->check = CSUM_MANGLED_0;
>> +
>>       skb->ip_summed = CHECKSUM_NONE;
>
>
> Shouldn't it be changed to CHECKSUM_PARTIAL or something ?
>
> udp4_ufo_send_check() was able to fill CHECKSUM_PARTIAL,
> I fear I do not understand enough this part.

Right, I had tried that also. Didn't work, IIRC there was a crash
later on because csum_start wasn't initialized.


>
>
>
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/3] net: Eliminate gso_send_check
  2014-09-20 21:52 [PATCH net-next 0/3] net: Eliminate gso_send_check Tom Herbert
                   ` (3 preceding siblings ...)
  2014-09-21  1:04 ` [PATCH net-next 0/3] net: Eliminate gso_send_check Eric Dumazet
@ 2014-09-26  4:23 ` David Miller
  4 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2014-09-26  4:23 UTC (permalink / raw)
  To: therbert; +Cc: netdev

From: Tom Herbert <therbert@google.com>
Date: Sat, 20 Sep 2014 14:52:27 -0700

> gso_send_check presents a lot of complexity for what it is being used
> for. It seems that there are only two cases where it might be effective:
> TCP and UFO paths. In these cases, the gso_send_check function
> initializes the TCP or UDP checksum respectively to the pseudo header
> checksum so that the checksum computation is appropriately offloaded or
> computed in the gso_segment functions. The gso_send_check functions
> are only called from dev.c in skb_mac_gso_segment when ip_summed !=
> CHECKSUM_PARTIAL (which seems very unlikely in TCP case). We can move
> the logic of this into the respective gso_segment functions where the
> checksum is initialized if ip_summed != CHECKSUM_PARTIAL.
> 
> With the above cases handled, gso_send_check is no longer needed, so
> we can remove all uses of it and the fields in the offload callbacks.
> With this change, ip_summed in the skb should be preserved though all
> the layers of gso_segment calls.
> 
> In follow-on patches, we may be able to remove the check setup code in
> tcp_gso_segment if we can guarantee that ip_summed will always be
> CHECKSUM_PARTIAL (verify all paths and probably add an assert in
> tcp_gro_segment).
> 
> Tested these patches by:
>   - netperf TCP_STREAM test with GSO enabled
>   - Forced ip_summed != CHECKSUM_PARTIAL with above
>   - Ran UDP_RR with 10000 request size over GRE tunnel. This exercised
>     UFO path.

Series applied, thanks Tom.

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-09-26  4:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-20 21:52 [PATCH net-next 0/3] net: Eliminate gso_send_check Tom Herbert
2014-09-20 21:52 ` [PATCH net-next 1/3] tcp: move logic out of tcp_v[64]_gso_send_check Tom Herbert
2014-09-21  1:04   ` Eric Dumazet
2014-09-20 21:52 ` [PATCH net-next 2/3] udp: move logic out of udp[46]_ufo_send_check Tom Herbert
2014-09-21  1:12   ` Eric Dumazet
2014-09-21  1:29     ` Tom Herbert
2014-09-20 21:52 ` [PATCH net-next 3/3] net: Remove gso_send_check as an offload callback Tom Herbert
2014-09-21  1:04 ` [PATCH net-next 0/3] net: Eliminate gso_send_check Eric Dumazet
2014-09-26  4:23 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).