Netdev List
 help / color / mirror / Atom feed
* [PATCH RFC 8/9] geneve: change to use UDP socket GRO
From: Tom Herbert @ 2016-03-23 22:36 UTC (permalink / raw)
  To: davem, netdev; +Cc: kernel-team
In-Reply-To: <1458772618-845742-1-git-send-email-tom@herbertland.com>

Adapt geneve_gro_receive, geneve_gro_complete to take a socket argument.
Set these functions in tunnel_config.  Don't set udp_offloads any more.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 drivers/net/geneve.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 192631a..6caf9ed 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -87,7 +87,6 @@ struct geneve_sock {
 	struct socket		*sock;
 	struct rcu_head		rcu;
 	int			refcnt;
-	struct udp_offload	udp_offloads;
 	struct hlist_head	vni_list[VNI_HASH_SIZE];
 	u32			flags;
 };
@@ -409,14 +408,6 @@ static void geneve_notify_add_rx_port(struct geneve_sock *gs)
 	struct net *net = sock_net(sk);
 	sa_family_t sa_family = geneve_get_sk_family(gs);
 	__be16 port = inet_sk(sk)->inet_sport;
-	int err;
-
-	if (sa_family == AF_INET) {
-		err = udp_add_offload(sock_net(sk), &gs->udp_offloads);
-		if (err)
-			pr_warn("geneve: udp_add_offload failed with status %d\n",
-				err);
-	}
 
 	rcu_read_lock();
 	for_each_netdev_rcu(net, dev) {
@@ -432,9 +423,9 @@ static int geneve_hlen(struct genevehdr *gh)
 	return sizeof(*gh) + gh->opt_len * 4;
 }
 
-static struct sk_buff **geneve_gro_receive(struct sk_buff **head,
-					   struct sk_buff *skb,
-					   struct udp_offload *uoff)
+static struct sk_buff **geneve_gro_receive(struct sock *sk,
+					   struct sk_buff **head,
+					   struct sk_buff *skb)
 {
 	struct sk_buff *p, **pp = NULL;
 	struct genevehdr *gh, *gh2;
@@ -495,8 +486,8 @@ out:
 	return pp;
 }
 
-static int geneve_gro_complete(struct sk_buff *skb, int nhoff,
-			       struct udp_offload *uoff)
+static int geneve_gro_complete(struct sock *sk, struct sk_buff *skb,
+			       int nhoff)
 {
 	struct genevehdr *gh;
 	struct packet_offload *ptype;
@@ -545,14 +536,15 @@ static struct geneve_sock *geneve_socket_create(struct net *net, __be16 port,
 		INIT_HLIST_HEAD(&gs->vni_list[h]);
 
 	/* Initialize the geneve udp offloads structure */
-	gs->udp_offloads.port = port;
-	gs->udp_offloads.callbacks.gro_receive  = geneve_gro_receive;
-	gs->udp_offloads.callbacks.gro_complete = geneve_gro_complete;
 	geneve_notify_add_rx_port(gs);
 
 	/* Mark socket as an encapsulation socket */
+	memset(&tunnel_cfg, 0, sizeof(tunnel_cfg));
 	tunnel_cfg.sk_user_data = gs;
 	tunnel_cfg.encap_type = 1;
+	tunnel_cfg.encap_fast = 0;
+	tunnel_cfg.gro_receive = geneve_gro_receive;
+	tunnel_cfg.gro_complete = geneve_gro_complete;
 	tunnel_cfg.encap_rcv = geneve_udp_encap_recv;
 	tunnel_cfg.encap_destroy = NULL;
 	setup_udp_tunnel_sock(net, sock, &tunnel_cfg);
@@ -576,9 +568,6 @@ static void geneve_notify_del_rx_port(struct geneve_sock *gs)
 	}
 
 	rcu_read_unlock();
-
-	if (sa_family == AF_INET)
-		udp_del_offload(&gs->udp_offloads);
 }
 
 static void __geneve_sock_release(struct geneve_sock *gs)
-- 
2.8.0.rc2

^ permalink raw reply related

* [PATCH RFC 1/9] net: Check skb_dst for NULL in inet_iif
From: Tom Herbert @ 2016-03-23 22:36 UTC (permalink / raw)
  To: davem, netdev; +Cc: kernel-team
In-Reply-To: <1458772618-845742-1-git-send-email-tom@herbertland.com>

In inet_iif check if skb_rtable is NULL for the skb and return
skb->skb_iif if it is.

This change allows inet_iif to be called before the dst
information has been set in the skb (e.g. when doing socket based
UDP GRO).

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/route.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/net/route.h b/include/net/route.h
index 9b0a523..f4b11ee 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -322,10 +322,11 @@ static inline struct rtable *ip_route_newports(struct flowi4 *fl4, struct rtable
 
 static inline int inet_iif(const struct sk_buff *skb)
 {
-	int iif = skb_rtable(skb)->rt_iif;
+	struct rtable *rt = skb_rtable(skb);
+
+	if (rt && rt->rt_iif)
+		return rt->rt_iif;
 
-	if (iif)
-		return iif;
 	return skb->skb_iif;
 }
 
-- 
2.8.0.rc2

^ permalink raw reply related

* [PATCH RFC 5/9] udp: Add socket based GRO and fast receive encap to tunnel config
From: Tom Herbert @ 2016-03-23 22:36 UTC (permalink / raw)
  To: davem, netdev; +Cc: kernel-team
In-Reply-To: <1458772618-845742-1-git-send-email-tom@herbertland.com>

Add gro_receive, gro_complete and encap_fast to
struct udp_tunnel_sock_cfg.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/udp_tunnel.h | 8 ++++++++
 net/ipv4/udp_tunnel.c    | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index b831140..3c534e7 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -64,13 +64,21 @@ static inline int udp_sock_create(struct net *net,
 
 typedef int (*udp_tunnel_encap_rcv_t)(struct sock *sk, struct sk_buff *skb);
 typedef void (*udp_tunnel_encap_destroy_t)(struct sock *sk);
+typedef struct sk_buff **(*udp_tunnel_gro_receive_t)(struct sock *sk,
+						     struct sk_buff **head,
+						     struct sk_buff *skb);
+typedef int (*udp_tunnel_gro_complete_t)(struct sock *sk, struct sk_buff *skb,
+					 int nhoff);
 
 struct udp_tunnel_sock_cfg {
 	void *sk_user_data;     /* user data used by encap_rcv call back */
 	/* Used for setting up udp_sock fields, see udp.h for details */
 	__u8  encap_type;
+	__u8 encap_fast:1;
 	udp_tunnel_encap_rcv_t encap_rcv;
 	udp_tunnel_encap_destroy_t encap_destroy;
+	udp_tunnel_gro_receive_t gro_receive;
+	udp_tunnel_gro_complete_t gro_complete;
 };
 
 /* Setup the given (UDP) sock to receive UDP encapsulated packets */
diff --git a/net/ipv4/udp_tunnel.c b/net/ipv4/udp_tunnel.c
index 96599d1..b5b13f8 100644
--- a/net/ipv4/udp_tunnel.c
+++ b/net/ipv4/udp_tunnel.c
@@ -69,6 +69,9 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
 	udp_sk(sk)->encap_type = cfg->encap_type;
 	udp_sk(sk)->encap_rcv = cfg->encap_rcv;
 	udp_sk(sk)->encap_destroy = cfg->encap_destroy;
+	udp_sk(sk)->encap_fast = cfg->encap_fast;
+	udp_sk(sk)->gro_receive = cfg->gro_receive;
+	udp_sk(sk)->gro_complete = cfg->gro_complete;
 
 	udp_tunnel_encap_enable(sock);
 }
-- 
2.8.0.rc2

^ permalink raw reply related

* [PATCH RFC 7/9] fou: change to use UDP socket GRO and fast rcv encap
From: Tom Herbert @ 2016-03-23 22:36 UTC (permalink / raw)
  To: davem, netdev; +Cc: kernel-team
In-Reply-To: <1458772618-845742-1-git-send-email-tom@herbertland.com>

Adapt gue_gro_receive, gue_gro_complete to take a socket argument.
Set the encap_fast flag on fou sockets. Don't set udp_offloads
any more.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 net/ipv4/fou.c | 49 ++++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
index 7804842..a90d179 100644
--- a/net/ipv4/fou.c
+++ b/net/ipv4/fou.c
@@ -22,7 +22,6 @@ struct fou {
 	u8 flags;
 	__be16 port;
 	u16 type;
-	struct udp_offload udp_offloads;
 	struct list_head list;
 	struct rcu_head rcu;
 };
@@ -177,13 +176,13 @@ drop:
 	return 0;
 }
 
-static struct sk_buff **fou_gro_receive(struct sk_buff **head,
-					struct sk_buff *skb,
-					struct udp_offload *uoff)
+static struct sk_buff **fou_gro_receive(struct sock *sk,
+					struct sk_buff **head,
+					struct sk_buff *skb)
 {
 	const struct net_offload *ops;
 	struct sk_buff **pp = NULL;
-	u8 proto = NAPI_GRO_CB(skb)->proto;
+	u8 proto = fou_from_sock(sk)->protocol;
 	const struct net_offload **offloads;
 
 	rcu_read_lock();
@@ -200,11 +199,11 @@ out_unlock:
 	return pp;
 }
 
-static int fou_gro_complete(struct sk_buff *skb, int nhoff,
-			    struct udp_offload *uoff)
+static int fou_gro_complete(struct sock *sk, struct sk_buff *skb,
+			    int nhoff)
 {
 	const struct net_offload *ops;
-	u8 proto = NAPI_GRO_CB(skb)->proto;
+	u8 proto = fou_from_sock(sk)->protocol;
 	int err = -ENOSYS;
 	const struct net_offload **offloads;
 
@@ -247,9 +246,9 @@ static struct guehdr *gue_gro_remcsum(struct sk_buff *skb, unsigned int off,
 	return guehdr;
 }
 
-static struct sk_buff **gue_gro_receive(struct sk_buff **head,
-					struct sk_buff *skb,
-					struct udp_offload *uoff)
+static struct sk_buff **gue_gro_receive(struct sock *sk,
+					struct sk_buff **head,
+					struct sk_buff *skb)
 {
 	const struct net_offload **offloads;
 	const struct net_offload *ops;
@@ -260,7 +259,7 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
 	void *data;
 	u16 doffset = 0;
 	int flush = 1;
-	struct fou *fou = container_of(uoff, struct fou, udp_offloads);
+	struct fou *fou = fou_from_sock(sk);
 	struct gro_remcsum grc;
 
 	skb_gro_remcsum_init(&grc);
@@ -361,8 +360,7 @@ out:
 	return pp;
 }
 
-static int gue_gro_complete(struct sk_buff *skb, int nhoff,
-			    struct udp_offload *uoff)
+static int gue_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff)
 {
 	const struct net_offload **offloads;
 	struct guehdr *guehdr = (struct guehdr *)(skb->data + nhoff);
@@ -410,10 +408,7 @@ static int fou_add_to_port_list(struct net *net, struct fou *fou)
 static void fou_release(struct fou *fou)
 {
 	struct socket *sock = fou->sock;
-	struct sock *sk = sock->sk;
 
-	if (sk->sk_family == AF_INET)
-		udp_del_offload(&fou->udp_offloads);
 	list_del(&fou->list);
 	udp_tunnel_sock_release(sock);
 
@@ -422,12 +417,10 @@ static void fou_release(struct fou *fou)
 
 static int fou_encap_init(struct sock *sk, struct fou *fou, struct fou_cfg *cfg)
 {
-	udp_sk(sk)->encap_rcv = fou_udp_recv;
 	fou->protocol = cfg->protocol;
-	fou->udp_offloads.callbacks.gro_receive = fou_gro_receive;
-	fou->udp_offloads.callbacks.gro_complete = fou_gro_complete;
-	fou->udp_offloads.port = cfg->udp_config.local_udp_port;
-	fou->udp_offloads.ipproto = cfg->protocol;
+	udp_sk(sk)->encap_rcv = fou_udp_recv;
+	udp_sk(sk)->gro_receive = fou_gro_receive;
+	udp_sk(sk)->gro_complete = fou_gro_complete;
 
 	return 0;
 }
@@ -435,9 +428,8 @@ static int fou_encap_init(struct sock *sk, struct fou *fou, struct fou_cfg *cfg)
 static int gue_encap_init(struct sock *sk, struct fou *fou, struct fou_cfg *cfg)
 {
 	udp_sk(sk)->encap_rcv = gue_udp_recv;
-	fou->udp_offloads.callbacks.gro_receive = gue_gro_receive;
-	fou->udp_offloads.callbacks.gro_complete = gue_gro_complete;
-	fou->udp_offloads.port = cfg->udp_config.local_udp_port;
+	udp_sk(sk)->gro_receive = gue_gro_receive;
+	udp_sk(sk)->gro_complete = gue_gro_complete;
 
 	return 0;
 }
@@ -487,6 +479,7 @@ static int fou_create(struct net *net, struct fou_cfg *cfg,
 	fou->type = cfg->type;
 
 	udp_sk(sk)->encap_type = 1;
+	udp_sk(sk)->encap_fast = 1;
 	udp_encap_enable();
 
 	sk->sk_user_data = fou;
@@ -496,12 +489,6 @@ static int fou_create(struct net *net, struct fou_cfg *cfg,
 
 	sk->sk_allocation = GFP_ATOMIC;
 
-	if (cfg->udp_config.family == AF_INET) {
-		err = udp_add_offload(net, &fou->udp_offloads);
-		if (err)
-			goto error;
-	}
-
 	err = fou_add_to_port_list(net, fou);
 	if (err)
 		goto error;
-- 
2.8.0.rc2

^ permalink raw reply related

* [PATCH RFC 2/9] udp: Add noreference lookup functions
From: Tom Herbert @ 2016-03-23 22:36 UTC (permalink / raw)
  To: davem, netdev; +Cc: kernel-team
In-Reply-To: <1458772618-845742-1-git-send-email-tom@herbertland.com>

This patches adds udp6_lib_lookup_skb_noref, udp4_lib_lookup_skb_noref
and related support to allow a caller to lookup a UDP socket
without automatically taking a reference. The lookup and caller
use of the socket must be done under rcu_read_lock.

This feature will be used in a fast receive encapsulation path and
also when performing GRO through callout in the UDP socket.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/udp.h | 12 ++++++++
 net/ipv4/udp.c    | 88 +++++++++++++++++++++++++++++++++++++++----------------
 net/ipv6/udp.c    | 85 ++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 136 insertions(+), 49 deletions(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index 92927f7..2a6f7b2 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -260,6 +260,8 @@ struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
 struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
 			       __be32 daddr, __be16 dport, int dif,
 			       struct udp_table *tbl, struct sk_buff *skb);
+struct sock *udp4_lib_lookup_skb_noref(struct sk_buff *skb,
+				       __be16 sport, __be16 dport);
 struct sock *udp6_lib_lookup(struct net *net,
 			     const struct in6_addr *saddr, __be16 sport,
 			     const struct in6_addr *daddr, __be16 dport,
@@ -269,6 +271,16 @@ struct sock *__udp6_lib_lookup(struct net *net,
 			       const struct in6_addr *daddr, __be16 dport,
 			       int dif, struct udp_table *tbl,
 			       struct sk_buff *skb);
+struct sock *udp6_lib_lookup_skb_noref(struct sk_buff *skb,
+				       __be16 sport, __be16 dport);
+
+static inline struct sock *udp_get_ref(struct sock *sk)
+{
+	if (unlikely(!atomic_inc_not_zero_hint(&sk->sk_refcnt, 2)))
+		return NULL;
+
+	return sk;
+}
 
 /*
  * 	SNMP statistics for UDP and UDP-Lite
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 836abe5..324d008 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -494,7 +494,7 @@ static struct sock *udp4_lib_lookup2(struct net *net,
 		__be32 saddr, __be16 sport,
 		__be32 daddr, unsigned int hnum, int dif,
 		struct udp_hslot *hslot2, unsigned int slot2,
-		struct sk_buff *skb)
+		struct sk_buff *skb, bool get_ref)
 {
 	struct sock *sk, *result;
 	struct hlist_nulls_node *node;
@@ -544,12 +544,14 @@ begin:
 		goto begin;
 	if (result) {
 found:
-		if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
-			result = NULL;
-		else if (unlikely(compute_score2(result, net, saddr, sport,
-				  daddr, hnum, dif) < badness)) {
-			sock_put(result);
-			goto begin;
+		if (get_ref) {
+			if (!udp_get_ref(result)) {
+				result = NULL;
+			} else if (unlikely(compute_score2(result, net, saddr,
+					  sport, daddr, hnum, dif) < badness)) {
+				sock_put(result);
+				goto begin;
+			}
 		}
 	}
 	return result;
@@ -558,9 +560,11 @@ found:
 /* UDP is nearly always wildcards out the wazoo, it makes no sense to try
  * harder than this. -DaveM
  */
-struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
+/* called with read_rcu_lock() */
+static struct sock *___udp4_lib_lookup(struct net *net, __be32 saddr,
 		__be16 sport, __be32 daddr, __be16 dport,
-		int dif, struct udp_table *udptable, struct sk_buff *skb)
+		int dif, struct udp_table *udptable, struct sk_buff *skb,
+		bool get_ref)
 {
 	struct sock *sk, *result;
 	struct hlist_nulls_node *node;
@@ -571,7 +575,6 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 	bool select_ok = true;
 	u32 hash = 0;
 
-	rcu_read_lock();
 	if (hslot->count > 10) {
 		hash2 = udp4_portaddr_hash(net, daddr, hnum);
 		slot2 = hash2 & udptable->mask;
@@ -581,7 +584,7 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 
 		result = udp4_lib_lookup2(net, saddr, sport,
 					  daddr, hnum, dif,
-					  hslot2, slot2, skb);
+					  hslot2, slot2, skb, get_ref);
 		if (!result) {
 			hash2 = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum);
 			slot2 = hash2 & udptable->mask;
@@ -591,9 +594,8 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 
 			result = udp4_lib_lookup2(net, saddr, sport,
 						  htonl(INADDR_ANY), hnum, dif,
-						  hslot2, slot2, skb);
+						  hslot2, slot2, skb, get_ref);
 		}
-		rcu_read_unlock();
 		return result;
 	}
 begin:
@@ -639,19 +641,43 @@ begin:
 
 	if (result) {
 found:
-		if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
-			result = NULL;
-		else if (unlikely(compute_score(result, net, saddr, hnum, sport,
-				  daddr, dport, dif) < badness)) {
-			sock_put(result);
-			goto begin;
+		if (get_ref) {
+			if (!udp_get_ref(result)) {
+				result = NULL;
+			} else if (unlikely(compute_score(result, net, saddr,
+					  hnum, sport, daddr, dport,
+					  dif) < badness)) {
+				sock_put(result);
+				goto begin;
+			}
 		}
 	}
-	rcu_read_unlock();
 	return result;
 }
+
+struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
+		__be16 sport, __be32 daddr, __be16 dport,
+		int dif, struct udp_table *udptable, struct sk_buff *skb)
+{
+	struct sock *sk;
+
+	rcu_read_lock();
+	sk = ___udp4_lib_lookup(net, saddr, sport, daddr, dport, dif, udptable,
+				skb, true);
+	rcu_read_unlock();
+
+	return sk;
+}
 EXPORT_SYMBOL_GPL(__udp4_lib_lookup);
 
+struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
+			     __be32 daddr, __be16 dport, int dif)
+{
+	return __udp4_lib_lookup(net, saddr, sport, daddr, dport, dif,
+				 &udp_table, NULL);
+}
+EXPORT_SYMBOL_GPL(udp4_lib_lookup);
+
 static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
 						 __be16 sport, __be16 dport,
 						 struct udp_table *udptable)
@@ -663,13 +689,24 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
 				 udptable, skb);
 }
 
-struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
-			     __be32 daddr, __be16 dport, int dif)
+static inline struct sock *__udp4_lib_lookup_skb_noref(struct sk_buff *skb,
+					__be16 sport, __be16 dport,
+					struct udp_table *udptable)
 {
-	return __udp4_lib_lookup(net, saddr, sport, daddr, dport, dif,
-				 &udp_table, NULL);
+	const struct iphdr *iph = ip_hdr(skb);
+	struct net_device *dev = skb_dst(skb) ? skb_dst(skb)->dev : skb->dev;
+
+	return ___udp4_lib_lookup(dev_net(dev), iph->saddr, sport,
+				  iph->daddr, dport, inet_iif(skb),
+				  udptable, skb, false);
 }
-EXPORT_SYMBOL_GPL(udp4_lib_lookup);
+
+struct sock *udp4_lib_lookup_skb_noref(struct sk_buff *skb,
+				       __be16 sport, __be16 dport)
+{
+	return __udp4_lib_lookup_skb_noref(skb, sport, dport, &udp_table);
+}
+EXPORT_SYMBOL_GPL(udp4_lib_lookup_skb_noref);
 
 static inline bool __udp_is_mcast_sock(struct net *net, struct sock *sk,
 				       __be16 loc_port, __be32 loc_addr,
@@ -1563,7 +1600,6 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	}
 
 	return 0;
-
 }
 
 static struct static_key udp_encap_needed __read_mostly;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index fd25e44..281469c 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -210,7 +210,7 @@ static struct sock *udp6_lib_lookup2(struct net *net,
 		const struct in6_addr *saddr, __be16 sport,
 		const struct in6_addr *daddr, unsigned int hnum, int dif,
 		struct udp_hslot *hslot2, unsigned int slot2,
-		struct sk_buff *skb)
+		struct sk_buff *skb, bool get_ref)
 {
 	struct sock *sk, *result;
 	struct hlist_nulls_node *node;
@@ -261,22 +261,25 @@ begin:
 
 	if (result) {
 found:
-		if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
-			result = NULL;
-		else if (unlikely(compute_score2(result, net, saddr, sport,
-				  daddr, hnum, dif) < badness)) {
-			sock_put(result);
-			goto begin;
+		if (get_ref) {
+			if (!udp_get_ref(result)) {
+				result = NULL;
+			} else if (unlikely(compute_score2(result, net, saddr,
+					  sport, daddr, hnum, dif) < badness)) {
+				sock_put(result);
+				goto begin;
+			}
 		}
 	}
 	return result;
 }
 
-struct sock *__udp6_lib_lookup(struct net *net,
-				      const struct in6_addr *saddr, __be16 sport,
-				      const struct in6_addr *daddr, __be16 dport,
-				      int dif, struct udp_table *udptable,
-				      struct sk_buff *skb)
+/* called with read_rcu_lock() */
+struct sock *___udp6_lib_lookup(struct net *net,
+				const struct in6_addr *saddr, __be16 sport,
+				const struct in6_addr *daddr, __be16 dport,
+				int dif, struct udp_table *udptable,
+				struct sk_buff *skb, bool get_ref)
 {
 	struct sock *sk, *result;
 	struct hlist_nulls_node *node;
@@ -287,7 +290,6 @@ struct sock *__udp6_lib_lookup(struct net *net,
 	bool select_ok = true;
 	u32 hash = 0;
 
-	rcu_read_lock();
 	if (hslot->count > 10) {
 		hash2 = udp6_portaddr_hash(net, daddr, hnum);
 		slot2 = hash2 & udptable->mask;
@@ -297,7 +299,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
 
 		result = udp6_lib_lookup2(net, saddr, sport,
 					  daddr, hnum, dif,
-					  hslot2, slot2, skb);
+					  hslot2, slot2, skb, get_ref);
 		if (!result) {
 			hash2 = udp6_portaddr_hash(net, &in6addr_any, hnum);
 			slot2 = hash2 & udptable->mask;
@@ -307,9 +309,8 @@ struct sock *__udp6_lib_lookup(struct net *net,
 
 			result = udp6_lib_lookup2(net, saddr, sport,
 						  &in6addr_any, hnum, dif,
-						  hslot2, slot2, skb);
+						  hslot2, slot2, skb, get_ref);
 		}
-		rcu_read_unlock();
 		return result;
 	}
 begin:
@@ -354,17 +355,35 @@ begin:
 
 	if (result) {
 found:
-		if (unlikely(!atomic_inc_not_zero_hint(&result->sk_refcnt, 2)))
-			result = NULL;
-		else if (unlikely(compute_score(result, net, hnum, saddr, sport,
-					daddr, dport, dif) < badness)) {
-			sock_put(result);
-			goto begin;
+		if (get_ref) {
+			if (!udp_get_ref(result)) {
+				result = NULL;
+			} else if (unlikely(compute_score(result, net, hnum,
+						saddr, sport, daddr, dport,
+						dif) < badness)) {
+				sock_put(result);
+				goto begin;
+			}
 		}
 	}
-	rcu_read_unlock();
 	return result;
 }
+
+struct sock *__udp6_lib_lookup(struct net *net,
+			       const struct in6_addr *saddr, __be16 sport,
+			       const struct in6_addr *daddr, __be16 dport,
+			       int dif, struct udp_table *udptable,
+			       struct sk_buff *skb)
+{
+	struct sock *sk;
+
+	rcu_read_lock();
+	sk = ___udp6_lib_lookup(net, saddr, sport, daddr, dport, dif,
+				udptable, skb, true);
+	rcu_read_unlock();
+
+	return sk;
+}
 EXPORT_SYMBOL_GPL(__udp6_lib_lookup);
 
 static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb,
@@ -389,6 +408,26 @@ struct sock *udp6_lib_lookup(struct net *net, const struct in6_addr *saddr, __be
 }
 EXPORT_SYMBOL_GPL(udp6_lib_lookup);
 
+static inline struct sock *__udp6_lib_lookup_skb_noref(struct sk_buff *skb,
+						       __be16 sport,
+						       __be16 dport,
+						       struct udp_table *udptable)
+{
+	const struct ipv6hdr *iph = ipv6_hdr(skb);
+	struct net_device *dev = skb_dst(skb) ? skb_dst(skb)->dev : skb->dev;
+
+	return ___udp6_lib_lookup(dev_net(dev), &iph->saddr, sport,
+				  &iph->daddr, dport, inet6_iif(skb),
+				  udptable, skb, false);
+}
+
+struct sock *udp6_lib_lookup_skb_noref(struct sk_buff *skb, __be16 sport,
+				       __be16 dport)
+{
+	return __udp6_lib_lookup_skb_noref(skb, sport, dport, &udp_table);
+}
+EXPORT_SYMBOL(udp6_lib_lookup_skb_noref);
+
 /*
  *	This should be easy, if there is something there we
  *	return it, otherwise we block.
-- 
2.8.0.rc2

^ permalink raw reply related

* [PATCH RFC 9/9] udp: Remove udp_offloads
From: Tom Herbert @ 2016-03-23 22:36 UTC (permalink / raw)
  To: davem, netdev; +Cc: kernel-team
In-Reply-To: <1458772618-845742-1-git-send-email-tom@herbertland.com>

Now that the UDP encapsulation GRO functions have been moved to the UDP
socket we not longer need the udp_offload insfrastructure so removing it.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/linux/netdevice.h | 17 -------------
 include/net/protocol.h    |  3 ---
 net/ipv4/udp_offload.c    | 63 -----------------------------------------------
 3 files changed, 83 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be693b3..9262ac9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2152,23 +2152,6 @@ struct packet_offload {
 	struct list_head	 list;
 };
 
-struct udp_offload;
-
-struct udp_offload_callbacks {
-	struct sk_buff		**(*gro_receive)(struct sk_buff **head,
-						 struct sk_buff *skb,
-						 struct udp_offload *uoff);
-	int			(*gro_complete)(struct sk_buff *skb,
-						int nhoff,
-						struct udp_offload *uoff);
-};
-
-struct udp_offload {
-	__be16			 port;
-	u8			 ipproto;
-	struct udp_offload_callbacks callbacks;
-};
-
 /* often modified stats are per cpu, other are shared (netdev->stats) */
 struct pcpu_sw_netstats {
 	u64     rx_packets;
diff --git a/include/net/protocol.h b/include/net/protocol.h
index da689f5..bf36ca3 100644
--- a/include/net/protocol.h
+++ b/include/net/protocol.h
@@ -107,9 +107,6 @@ int inet_del_offload(const struct net_offload *prot, unsigned char num);
 void inet_register_protosw(struct inet_protosw *p);
 void inet_unregister_protosw(struct inet_protosw *p);
 
-int  udp_add_offload(struct net *net, struct udp_offload *prot);
-void udp_del_offload(struct udp_offload *prot);
-
 #if IS_ENABLED(CONFIG_IPV6)
 int inet6_add_protocol(const struct inet6_protocol *prot, unsigned char num);
 int inet6_del_protocol(const struct inet6_protocol *prot, unsigned char num);
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index dce43c9..320c91e 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -14,18 +14,6 @@
 #include <net/udp.h>
 #include <net/protocol.h>
 
-static DEFINE_SPINLOCK(udp_offload_lock);
-static struct udp_offload_priv __rcu *udp_offload_base __read_mostly;
-
-#define udp_deref_protected(X) rcu_dereference_protected(X, lockdep_is_held(&udp_offload_lock))
-
-struct udp_offload_priv {
-	struct udp_offload	*offload;
-	possible_net_t	net;
-	struct rcu_head		rcu;
-	struct udp_offload_priv __rcu *next;
-};
-
 static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
 	netdev_features_t features,
 	struct sk_buff *(*gso_inner_segment)(struct sk_buff *skb,
@@ -254,56 +242,6 @@ out:
 	return segs;
 }
 
-int udp_add_offload(struct net *net, struct udp_offload *uo)
-{
-	struct udp_offload_priv *new_offload = kzalloc(sizeof(*new_offload), GFP_ATOMIC);
-
-	if (!new_offload)
-		return -ENOMEM;
-
-	write_pnet(&new_offload->net, net);
-	new_offload->offload = uo;
-
-	spin_lock(&udp_offload_lock);
-	new_offload->next = udp_offload_base;
-	rcu_assign_pointer(udp_offload_base, new_offload);
-	spin_unlock(&udp_offload_lock);
-
-	return 0;
-}
-EXPORT_SYMBOL(udp_add_offload);
-
-static void udp_offload_free_routine(struct rcu_head *head)
-{
-	struct udp_offload_priv *ou_priv = container_of(head, struct udp_offload_priv, rcu);
-	kfree(ou_priv);
-}
-
-void udp_del_offload(struct udp_offload *uo)
-{
-	struct udp_offload_priv __rcu **head = &udp_offload_base;
-	struct udp_offload_priv *uo_priv;
-
-	spin_lock(&udp_offload_lock);
-
-	uo_priv = udp_deref_protected(*head);
-	for (; uo_priv != NULL;
-	     uo_priv = udp_deref_protected(*head)) {
-		if (uo_priv->offload == uo) {
-			rcu_assign_pointer(*head,
-					   udp_deref_protected(uo_priv->next));
-			goto unlock;
-		}
-		head = &uo_priv->next;
-	}
-	pr_warn("udp_del_offload: didn't find offload for port %d\n", ntohs(uo->port));
-unlock:
-	spin_unlock(&udp_offload_lock);
-	if (uo_priv)
-		call_rcu(&uo_priv->rcu, udp_offload_free_routine);
-}
-EXPORT_SYMBOL(udp_del_offload);
-
 struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 				 struct udphdr *uh, udp_lookup_t lookup)
 {
@@ -327,7 +265,6 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 
 	if (sk && udp_sk(sk)->gro_receive)
 		goto unflush;
-
 	goto out_unlock;
 
 unflush:
-- 
2.8.0.rc2

^ permalink raw reply related

* Re: [PATCH RFC 2/9] udp: Add noreference lookup functions
From: Eric Dumazet @ 2016-03-23 22:59 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, netdev, kernel-team
In-Reply-To: <1458772618-845742-3-git-send-email-tom@herbertland.com>

On Wed, 2016-03-23 at 15:36 -0700, Tom Herbert wrote:
> This patches adds udp6_lib_lookup_skb_noref, udp4_lib_lookup_skb_noref
> and related support to allow a caller to lookup a UDP socket
> without automatically taking a reference. The lookup and caller
> use of the socket must be done under rcu_read_lock.
> 
> This feature will be used in a fast receive encapsulation path and
> also when performing GRO through callout in the UDP socket.
> 
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> ---

As already mentioned in the past, you can not do a UDP lookup without
taking a reference, because we use SLAB_DESTROY_BY_RCU rules which are
strict.

Only taking a reference and re-doing the keys check can make sure the
lookup result makes sense (as we check multiple words and there is no
way it can be done atomically)

This is explained in include/linux/slab.h and
Documentation/RCU/rculist_nulls.txt

If you want that to happen, we need first to not use
SLAB_DESTROY_BY_RCU.

Then, we do not need to take a reference at all, even for the regular
UDP stack unicast receive path.

^ permalink raw reply

* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Edward Cree @ 2016-03-23 23:00 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Or Gerlitz, Alexander Duyck, Netdev, David Miller, Tom Herbert
In-Reply-To: <CAKgT0UfgKwSRjMwRHbDNawAyJ3jvz937j1jxGvJWRaD39bSSeQ@mail.gmail.com>

On 23/03/16 22:36, Alexander Duyck wrote:
> On Wed, Mar 23, 2016 at 2:05 PM, Edward Cree <ecree@solarflare.com> wrote:
>> I disagree.  Surely we should be able to "soft segment" the packet just
>> before we give it to the physical device, and then tell it to do dumb copying
>> of both the VXLAN and IPIP headers?  At this point, we don't have the problem
>> you identified above, because we've arrived at the device now.
> One issue here is that all levels of IP headers would have to have the
> DF bit set.  I don't think that happens right now.
Yes, that's still a requirement.  (Well, except for the outermost IP header.)
>> So we can chase through some per-protocol callbacks to shorten all the outer
>> lengths and adjust all the outer checksums, then hand it to the device for
>> TSO.  The device is treating the extra headers as an opaque blob, so it
>> doesn't know or care whether it's one layer of encapsulation or forty-two.
> So if we do pure software offloads this is doable.  However the GSO
> flags are meant to have hardware feature equivalents.  The problem is
> if you combine an IPIP and VXLAN header how do you know what header is
> what and which order things are in, and what is the likelihood of
> having a device that would get things right when dealing with 3 levels
> of IP headers.  This is one of the reasons why we don't support
> multiple levels of tunnels in the GSO code.  GSO is just meant to be a
> fall-back for hardware offloads.
Right, but if the hardware does things "the new way" it should work fine:
Packet still starts with Eth + IP.  Packet still has TCP headers at some
specified offset.  So it all works, as long as you don't have to update
any IP IDs except possibly the outermost one.
>> Ok, it sounds like the interface to Intel hardware is just Very Different
>> to Solarflare hardware on this point: we don't tell our hardware anything
>> about where the various headers start, it just parses them to figure it
>> out.  (And for new-style TSO we'd tell it where the TCP header starts, as
>> I described before.)
> That is kind of what I figured.  So does that mean for IPv6 you guys
> are parsing through extension headers?  I believe that is one of the
> reasons why Intel did things the way they did is to avoid having to
> parse through any IPv4 options or IPv6 extension headers.
I believe so, but I'd have to check with our firmware team to be sure.
The hardware needs to have that capability for RX processing, where it
wants to figure out things like the l4proto for IPv6: you have to walk
the extension headers until you get a layer 4 nexthdr.  I wonder how
Intel manage without that?
>> I agree this isn't something we can do silently.  But we _can_ make it a
>> condition for enabling gso-partial.  And I think it's a necessary
>> condition for truly generic TSO.  Sure, your 'L3 extension header' works
>> fine for a single tunnel.  But if you nest tunnels, you now need to
>> update the outer _and_ middle IP IDs, and you can't do that because you
>> only have one L3 header pointer.
> This is getting away from the 'less is more' concept.  If we are doing
> multiple levels of tunnels we have already made things far too
> complicated and it is unlikely hardware will ever support anything
> like that.
That's not how I understood the concept.  I parsed it as "if hardware knows
less, we can get more out of it", i.e. by having the hardware blithely paste
together whatever headers you give it, you can support things like nested
tunnels.  As long as your 'middle' IP header has DF set, this can be done
without the hardware needing to know a thing about it.  And while we don't
need to implement that straight away, we should care to design our
interfaces to ensure we can do that in the future without too much trouble.
>> Of course, that means changing the firmware; luckily we haven't got any
>> parts in the wild doing tunnel offloads yet, so we still have a chance
>> to do that without needing driver code to work around our past
>> mistakes...
>>
>> But this stuff does definitely add value for us, it means we could TSO
>> any tunnel type whatsoever; even nested tunnels as long as only the
>> outermost IP ID needs to change.
> Right.  In your case it sounds like you would have the advantage of
> just having to run essentially two counters, one increments the IPv4
> ID and the other decrements the IPv4 checksum.  Beyond that the outer
> headers wouldn't need to change at all.
Exactly.
> The only other issue would be determining how the inner pseudo-header
> checksum is updated.  If you were parsing out header fields from the
> IP header previously to generate it you would instead need to update
> things so that you could use the partial checksum that is already
> stored in the TCP header checksum field.
Right, but again that's sufficiently under firmware control (AFAIK) that
that should just be a SMOP for the firmware.  Though I will ask about
that tomorrow, just in case.

-Ed

^ permalink raw reply

* Re: linux-next: manual merge of the rdma tree with the net-next tree
From: Or Gerlitz @ 2016-03-23 23:04 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Doug Ledford, Stephen Rothwell, David Miller, Network Development,
	linux-next, Linux Kernel Mailing List
In-Reply-To: <CA+55aFyi31eRO6nO1-kz=-kq4XqyyA=VagbsiaO4ZPu1aLmnuA@mail.gmail.com>

On Wed, Mar 16, 2016 at 7:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Mar 16, 2016 at 10:35 AM, Doug Ledford <dledford@redhat.com> wrote:
>> On 3/16/2016 1:18 PM, Linus Torvalds wrote:
>>> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:

>>>> I fixed it up (see below) and can carry the fix as necessary (no action
>>>> is required).

>>> Side note: can you change this wording for your manual merge script?
>>> Last merge window (or was it the one before it?) we had confusion with
>>> people who thought that "no action is required" means "you can just
>>> ignore this entirely".

>> I certainly didn't take it that way regardless of the wording.

> It was Or Gerlitz. You were cc'd, since it was the whole rdma Mellanox
> mess. I quote from that thread:
>
>  "> However, the fact that it got resolved in linux-next is purely
>   > informational. It doesn't "fix" the conflict - it just means that both
>   > sides should have gotten informed about it. That doesn't mean that the
>   > conflict goes away or becomes better.
>
>   That's news to me. When such things happen and caught by Stephen, we
>   are getting an email saying something like
>
>   "Today's linux-next merge of the infiniband tree got a conflict
>   between commit X from net-next tree and commit Y from the infiniband
>   tree. I fixed it up (see below) and can carry the fix as necessary (no
>   action is required)."
>
>   Also asked around a bit and got to learn on Stephen using git rerere,
>   so all (no action needed note + seeing git rerere in action...) that
>   leaded me to think that indeed no action is required from our side,
>   but after reading your email (twice, so far), I realized that this was
>   wrong conclusion."

> So that whole "no action is required" wording very much has caused
> confusion before in the rdma camp.

> Let's fix the wording. I'm indeed hopeful that the rdma camp is now
> keenly aware of the issues, but that doesn't change the fact that the
> wording has been problematic.

>> "[...] The Mellanox people are on my xxit-list until they show that they can
>> actually act like responsible people [...]"

Linus,

As I wrote you in that other thread you were quoting from there,
following to the happenings mentioned there, we took responsibility
and made bunch of corrective actions within Mellanox which got us to a
point where there was only one rdma/netdev-next conflict for the 4.6
merge window.

I know there's history here, and in the 4.5 cycle things were much
worse, but I still wanted to put things in their more precise place,
if you don't mind.

Or.

^ permalink raw reply

* Re: [PATCH v8 net-next] ravb: Add dma queue interrupt support
From: Yoshihiro Kaneko @ 2016-03-23 23:07 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, Sergei Shtylyov, Simon Horman, Magnus Damm,
	linux-renesas-soc
In-Reply-To: <20160322.155555.77700411133852493.davem@redhat.com>

2016-03-23 4:55 GMT+09:00 David Miller <davem@redhat.com>:
> From: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> Date: Wed, 23 Mar 2016 00:22:00 +0900
>
>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>
>> This patch supports the following interrupts.
>>
>> - One interrupt for multiple (timestamp, error, gPTP)
>> - One interrupt for emac
>> - Four interrupts for dma queue (best effort rx/tx, network control rx/tx)
>>
>> This patch improve efficiency of the interrupt handler by adding the
>> interrupt handler corresponding to each interrupt source described
>> above. Additionally, it reduces the number of times of the access to
>> EthernetAVB IF.
>> Also this patch prevent this driver depends on the whim of a boot loader.
>>
>> [ykaneko0929@gmail.com: define bit names of registers]
>> [ykaneko0929@gmail.com: add comment for gen3 only registers]
>> [ykaneko0929@gmail.com: fix coding style]
>> [ykaneko0929@gmail.com: update changelog]
>> [ykaneko0929@gmail.com: gen3: fix initialization of interrupts]
>> [ykaneko0929@gmail.com: gen3: fix clearing interrupts]
>> [ykaneko0929@gmail.com: gen3: add helper function for request_irq()]
>> [ykaneko0929@gmail.com: gen3: remove IRQF_SHARED flag for request_irq()]
>> [ykaneko0929@gmail.com: revert ravb_close() and ravb_ptp_stop()]
>> [ykaneko0929@gmail.com: avoid calling free_irq() to non-hooked interrupts]
>> [ykaneko0929@gmail.com: make NC/BE interrupt handler a function]
>> [ykaneko0929@gmail.com: make timestamp interrupt handler a function]
>> [ykaneko0929@gmail.com: timestamp interrupt is handled in multiple
>>  interrupt handler instead of dma queue interrupt handler]
>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
>
> Sorry, it is not appropriate to submit new features and major optimizations
> at this time.
>
> Please wait until some reasonable time after the merge window closes to
> resubmit this.

I'm sorry to bother you.
I will re-post this patch after net-next reopened.

>
> Thanks.

Best regards,
kaneko

^ permalink raw reply

* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Alexander Duyck @ 2016-03-23 23:15 UTC (permalink / raw)
  To: Edward Cree
  Cc: Or Gerlitz, Alexander Duyck, Netdev, David Miller, Tom Herbert
In-Reply-To: <56F3200C.20200@solarflare.com>

On Wed, Mar 23, 2016 at 4:00 PM, Edward Cree <ecree@solarflare.com> wrote:
> On 23/03/16 22:36, Alexander Duyck wrote:
>> On Wed, Mar 23, 2016 at 2:05 PM, Edward Cree <ecree@solarflare.com> wrote:
>>> I disagree.  Surely we should be able to "soft segment" the packet just
>>> before we give it to the physical device, and then tell it to do dumb copying
>>> of both the VXLAN and IPIP headers?  At this point, we don't have the problem
>>> you identified above, because we've arrived at the device now.
>> One issue here is that all levels of IP headers would have to have the
>> DF bit set.  I don't think that happens right now.
> Yes, that's still a requirement.  (Well, except for the outermost IP header.)
>>> So we can chase through some per-protocol callbacks to shorten all the outer
>>> lengths and adjust all the outer checksums, then hand it to the device for
>>> TSO.  The device is treating the extra headers as an opaque blob, so it
>>> doesn't know or care whether it's one layer of encapsulation or forty-two.
>> So if we do pure software offloads this is doable.  However the GSO
>> flags are meant to have hardware feature equivalents.  The problem is
>> if you combine an IPIP and VXLAN header how do you know what header is
>> what and which order things are in, and what is the likelihood of
>> having a device that would get things right when dealing with 3 levels
>> of IP headers.  This is one of the reasons why we don't support
>> multiple levels of tunnels in the GSO code.  GSO is just meant to be a
>> fall-back for hardware offloads.

> Right, but if the hardware does things "the new way" it should work fine:
> Packet still starts with Eth + IP.  Packet still has TCP headers at some
> specified offset.  So it all works, as long as you don't have to update
> any IP IDs except possibly the outermost one.

Right, but the problem becomes how do you identify what tunnel wants
what.  So for example we could theoretically have a UDP tunnel in a
UDP with checksum.  How would we tell which one want to have the
checksum set and which one doesn't?  The fact is we cannot.  You are
looking too far ahead.  We haven't gotten to tunnel in tunnel yet.
The approach as it stands doesn't have any issues that necessarily
prevent that as long as the outer is the only IP ID that has to
increment, but we don't support anything like that now so we don't
need to worry about it too much.

>>> Ok, it sounds like the interface to Intel hardware is just Very Different
>>> to Solarflare hardware on this point: we don't tell our hardware anything
>>> about where the various headers start, it just parses them to figure it
>>> out.  (And for new-style TSO we'd tell it where the TCP header starts, as
>>> I described before.)
>> That is kind of what I figured.  So does that mean for IPv6 you guys
>> are parsing through extension headers?  I believe that is one of the
>> reasons why Intel did things the way they did is to avoid having to
>> parse through any IPv4 options or IPv6 extension headers.

> I believe so, but I'd have to check with our firmware team to be sure.
> The hardware needs to have that capability for RX processing, where it
> wants to figure out things like the l4proto for IPv6: you have to walk
> the extension headers until you get a layer 4 nexthdr.  I wonder how
> Intel manage without that?

They have some parsing in the Rx.  That is one of the reasons why
there was all the arguing about adding GENEVE port numbers a few
months ago.  They just don't make use of it in the Tx path with the
exception of the fm10k parts.

>>> I agree this isn't something we can do silently.  But we _can_ make it a
>>> condition for enabling gso-partial.  And I think it's a necessary
>>> condition for truly generic TSO.  Sure, your 'L3 extension header' works
>>> fine for a single tunnel.  But if you nest tunnels, you now need to
>>> update the outer _and_ middle IP IDs, and you can't do that because you
>>> only have one L3 header pointer.
>> This is getting away from the 'less is more' concept.  If we are doing
>> multiple levels of tunnels we have already made things far too
>> complicated and it is unlikely hardware will ever support anything
>> like that.

> That's not how I understood the concept.  I parsed it as "if hardware knows
> less, we can get more out of it", i.e. by having the hardware blithely paste
> together whatever headers you give it, you can support things like nested
> tunnels.  As long as your 'middle' IP header has DF set, this can be done
> without the hardware needing to know a thing about it.  And while we don't
> need to implement that straight away, we should care to design our
> interfaces to ensure we can do that in the future without too much trouble.

The design as is does nothing to prevent that.  One of the reasons why
I prefer to keep the outer IP ID incrementing is in order to support
that kind of concept.  Also it shields us a bit as we usually cannot
control the network between the tunnel endpoints since it is usually
traversing a WAN.  What we need to do though is go through and see if
we can get away with something like "if inner IP DF is set the outer
IP DF bit must be set" kind of logic for GRE and UDP tunnels.  If we
can push that then it will allow us to essentially fix all the tunnel
logic in one shot since TCP requires DF bit be set so all levels of
headers would have the DF bit set.

>>> Of course, that means changing the firmware; luckily we haven't got any
>>> parts in the wild doing tunnel offloads yet, so we still have a chance
>>> to do that without needing driver code to work around our past
>>> mistakes...
>>>
>>> But this stuff does definitely add value for us, it means we could TSO
>>> any tunnel type whatsoever; even nested tunnels as long as only the
>>> outermost IP ID needs to change.
>> Right.  In your case it sounds like you would have the advantage of
>> just having to run essentially two counters, one increments the IPv4
>> ID and the other decrements the IPv4 checksum.  Beyond that the outer
>> headers wouldn't need to change at all.
> Exactly.
>> The only other issue would be determining how the inner pseudo-header
>> checksum is updated.  If you were parsing out header fields from the
>> IP header previously to generate it you would instead need to update
>> things so that you could use the partial checksum that is already
>> stored in the TCP header checksum field.
> Right, but again that's sufficiently under firmware control (AFAIK) that
> that should just be a SMOP for the firmware.  Though I will ask about
> that tomorrow, just in case.

There shouldn't be much to it.  In the case of the Intel parts they
want the length cancelled out of the checksum by the driver and they
they fold it back in via hardware.  I would imagine that your hardware
could probably do something similar or may already be doing it since
the length has to be handled differently for IPv4 vs IPv6.

- Alex

^ permalink raw reply

* Re: [PATCH RFC 2/9] udp: Add noreference lookup functions
From: Tom Herbert @ 2016-03-23 23:17 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S. Miller, Linux Kernel Network Developers, Kernel Team
In-Reply-To: <1458773986.10868.54.camel@edumazet-glaptop3.roam.corp.google.com>

On Wed, Mar 23, 2016 at 3:59 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2016-03-23 at 15:36 -0700, Tom Herbert wrote:
>> This patches adds udp6_lib_lookup_skb_noref, udp4_lib_lookup_skb_noref
>> and related support to allow a caller to lookup a UDP socket
>> without automatically taking a reference. The lookup and caller
>> use of the socket must be done under rcu_read_lock.
>>
>> This feature will be used in a fast receive encapsulation path and
>> also when performing GRO through callout in the UDP socket.
>>
>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>> ---
>
> As already mentioned in the past, you can not do a UDP lookup without
> taking a reference, because we use SLAB_DESTROY_BY_RCU rules which are
> strict.
>
> Only taking a reference and re-doing the keys check can make sure the
> lookup result makes sense (as we check multiple words and there is no
> way it can be done atomically)
>
> This is explained in include/linux/slab.h and
> Documentation/RCU/rculist_nulls.txt
>
> If you want that to happen, we need first to not use
> SLAB_DESTROY_BY_RCU.
>
> Then, we do not need to take a reference at all, even for the regular
> UDP stack unicast receive path.
>
Thanks Eric. Do you think it is reasonable to not use
SLAB_DESTROY_BY_RCU for UDP since it probably has a much lower
allocation rate than what we'd see in TCP?

Tom

>
>

^ permalink raw reply

* Re: linux-next: manual merge of the rdma tree with the net-next tree
From: Linus Torvalds @ 2016-03-23 23:23 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: Doug Ledford, Stephen Rothwell, David Miller, Network Development,
	linux-next, Linux Kernel Mailing List
In-Reply-To: <CAJ3xEMgth+KpQdcGi3Y8s9_0GJrEESCiEU_8uFGWrMOLHwOdHA@mail.gmail.com>

On Wed, Mar 23, 2016 at 4:04 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>
> I know there's history here, and in the 4.5 cycle things were much
> worse, but I still wanted to put things in their more precise place,
> if you don't mind.

We'll see how things shape up in the future. Once bitten, twice shy,
as they say.

Please make sure that Mellanox will not be a pain going forward, and
everything will be forgiven/forgotten.

            Linus

^ permalink raw reply

* Re: [PATCH RFC 2/9] udp: Add noreference lookup functions
From: Eric Dumazet @ 2016-03-23 23:28 UTC (permalink / raw)
  To: Tom Herbert; +Cc: David S. Miller, Linux Kernel Network Developers, Kernel Team
In-Reply-To: <CALx6S35pPLoyY6hiOg1WKKe4FUATgJQncWvoxJe-Z2NoOMe2eA@mail.gmail.com>

On Wed, 2016-03-23 at 16:17 -0700, Tom Herbert wrote:
> Thanks Eric. Do you think it is reasonable to not use
> SLAB_DESTROY_BY_RCU for UDP since it probably has a much lower
> allocation rate than what we'd see in TCP?

This is absolutely reasonable ;)

^ permalink raw reply

* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Wei Wang @ 2016-03-23 23:57 UTC (permalink / raw)
  To: Eric Dumazet, Martin KaFai Lau, Cong Wang, Eric Dumazet
  Cc: Wei Wang, David Miller, Linux Kernel Network Developers
In-Reply-To: <1458689814.10868.29.camel@edumazet-glaptop3.roam.corp.google.com>

What about something like this:

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed44663..21b4102 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1394,6 +1394,19 @@ static void ip6_rt_update_pmtu(struct dst_entry
*dst, struct sock *sk,
    __ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu);
 }

+static void ip6_fill_in_flow(struct flowi6 *fl6,  struct net *net,
+                  struct sk_buff *skb, int oif, u32 mark)
+{
+   const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
+
+   memset(fl6, 0, sizeof(fl6));
+   fl6->flowi6_oif = oif;
+   fl6->flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
+   fl6->daddr = iph->daddr;
+   fl6->saddr = iph->saddr;
+   fl6->flowlabel = ip6_flowinfo(iph);
+}
+
 void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
             int oif, u32 mark)
 {
@@ -1401,13 +1414,7 @@ void ip6_update_pmtu(struct sk_buff *skb,
struct net *net, __be32 mtu,
    struct dst_entry *dst;
    struct flowi6 fl6;

-   memset(&fl6, 0, sizeof(fl6));
-   fl6.flowi6_oif = oif;
-   fl6.flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
-   fl6.daddr = iph->daddr;
-   fl6.saddr = iph->saddr;
-   fl6.flowlabel = ip6_flowinfo(iph);
-
+   ip6_fill_in_flow(&fl6, net, skb, oif, mark);
    dst = ip6_route_output(net, NULL, &fl6);
    if (!dst->error)
        __ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu));
@@ -1417,8 +1424,22 @@ EXPORT_SYMBOL_GPL(ip6_update_pmtu);

 void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
 {
-   ip6_update_pmtu(skb, sock_net(sk), mtu,
+   struct ipv6_pinfo *np = inet6_sk(sk);
+   struct dst_entry *dst_new;
+   struct flowi6 fl6;
+   struct net *net = sock_net(sk);
+
+   ip6_update_pmtu(skb, net, mtu,
+           sk->sk_bound_dev_if, sk->sk_mark);
+
+   if (sk->sk_state == TCP_ESTABLISHED &&
+       !sk_dst_check(sk, np->dst_cookie)) {
+       ip6_fill_in_flow(&fl6, net, skb,
            sk->sk_bound_dev_if, sk->sk_mark);
+       dst_new = ip6_route_output(net, NULL, &fl6);
+       if (!IS_ERR(dst_new))
+           ip6_dst_store(sk, dst_new, NULL, NULL);
+   }
 }
 EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);


Thanks.
Wei

On Tue, Mar 22, 2016 at 4:36 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-03-22 at 13:13 -0700, Cong Wang wrote:
>> On Tue, Mar 22, 2016 at 11:03 AM, Wei Wang <tracywwnj@gmail.com> wrote:
>> > Thanks Martin and Cong.
>> >
>> > I guess then we are going with the following fix in ip6_sk_update_pmtu():
>> > 1. call ip6_upate_pmtu() as it is
>> > 2. do a dst_check()
>> > 3. re-lookup() if it is invalid
>> > 4. and then do a ip6_dst_store()/dst_set
>>
>> Exactly, please try the attached patch. Note I did nothing more than a
>> compile test.
>>
>> Does it make sense to you now?
>
>
> Hard to reply on your patch as it was not inlined.
>
> 1) Lot of code duplication, for some reason I do not yet understand.
>
> ip6_sk_update_pmtu() and ip6_update_pmtu() will basically do the same
> thing...
>
> 2)
>
> +       if (sk->sk_state == TCP_ESTABLISHED)
> +               ip6_dst_store(sk, dst, &iph->daddr, &iph->saddr);
> +out:
>
>
> ip6_dst_store() will do :
>
> np->daddr_cache = daddr;  (&iph->daddr)
> np->saddr_cache = saddr;  (&iph->saddr)
>
> So when skb is freed, daddr_cache & saddr_cache point to freed data.
>
>
>
>

^ permalink raw reply related

* Re: [PATCH] net: Fix typos and whitespace.
From: Tom Herbert @ 2016-03-24  0:28 UTC (permalink / raw)
  To: David Miller; +Cc: bhelgaas, Linux Kernel Network Developers, LKML
In-Reply-To: <20160323.142703.2259993852905081527.davem@davemloft.net>

On Wed, Mar 23, 2016 at 11:27 AM, David Miller <davem@davemloft.net> wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Wed, 23 Mar 2016 08:45:30 -0500
>
>> Fix typos.  Capitalize CPU, NAPI, RCU consistently.  Align structure
>> indentation.  No functional change intended; only comment and whitespace
>> changes.
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Does not apply to the current 'net' tree, please respin.

Why is this for net and not net-next?

^ permalink raw reply

* Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption
From: Herbert Xu @ 2016-03-24  0:45 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Subash Abhinov Kasiviswanathan, 'Steffen Klassert',
	netdev, jeromes
In-Reply-To: <1458754165.10868.41.camel@edumazet-glaptop3.roam.corp.google.com>

On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote:
>
> OK, but before calling netif_rx() are we properly testing dev->flags
> IFF_UP status ?
> 
> Otherwise, we still allow packets being queued after flush_backlog() had
> been called.

That's the first thing enqueue_to_backlog tests.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply

* Re: [PATCH] net: Fix typos and whitespace.
From: David Miller @ 2016-03-24  0:55 UTC (permalink / raw)
  To: tom; +Cc: bhelgaas, netdev, linux-kernel
In-Reply-To: <CALx6S34KC2cGwvW0RPE_9CEABhrTOrbhkBYb5Eg3PpQ8rspsAg@mail.gmail.com>

From: Tom Herbert <tom@herbertland.com>
Date: Wed, 23 Mar 2016 17:28:20 -0700

> On Wed, Mar 23, 2016 at 11:27 AM, David Miller <davem@davemloft.net> wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>> Date: Wed, 23 Mar 2016 08:45:30 -0500
>>
>>> Fix typos.  Capitalize CPU, NAPI, RCU consistently.  Align structure
>>> indentation.  No functional change intended; only comment and whitespace
>>> changes.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> Does not apply to the current 'net' tree, please respin.
> 
> Why is this for net and not net-next?

Fixing comment typos is always appropriate, as they enhance
understanding of the code and make it easier to maintain.

^ permalink raw reply

* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Jesse Gross @ 2016-03-24  1:37 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Tom Herbert, Alexander Duyck, Edward Cree,
	Linux Kernel Network Developers, David S. Miller
In-Reply-To: <CAKgT0Uc9FNJ59529V8=0vtZnVij+JrZ2s-uOpFjnvbiqNHdVYQ@mail.gmail.com>

On Wed, Mar 23, 2016 at 11:19 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Wed, Mar 23, 2016 at 10:09 AM, Tom Herbert <tom@herbertland.com> wrote:
>> Can you add some description about strategy for dealing with ip_id?
>
> Yeah.  I still need to add more documentation.  I just didn't want to
> get into details on it until we have finalized things a bit more.  I'm
> still wondering if we should follow the pattern of ipip tunnels in
> terms of setting the DF bit if the inner header has the DF bit set.
> If we end up needing to add code to do that then it makes it so that
> the ip_id value can be fixed for both inner and outer which makes the
> segmentation much simpler since the only header that would ever really
> need to be updated would be the transport header in order to get the
> checksum correct.

I tried to do this years ago but in practice it broke things.

There's enough middleboxes/firewalls/etc. out there that filter ICMP
messages that path MTU discovery isn't necessarily reliable. And while
you might argue that if the box is breaking things then the same would
be true for the original, unencapsulated TCP stream but a lot of times
there are some other hacks built in (like MSS clamping) that make
assumptions that the traffic is TCP. So at the minimum it is generally
good to have an option to force the DF bit off.

That being said, I actually think that it is good to have the DF bit
on by default for encapsulation headers being added. Unintentional
(and perhaps multiple layers of) fragmentation usually results in
unuseably bad performance and so it best to try to correct it,
hopefully automatically in most cases. And, of course, this is the
direction that IPv6 has already gone. If we can assume that this is
the most common case then in practice we can keep the outer headers
constant for the high performance path.

To me, incrementing the inner IP really seems the best choice. The
inner header is most likely someone else's traffic so it best to not
mess with that whereas the outer headers are likely ours and we know
the parameters for them (and can set the DF bit as we believe is
correct). Also, if you are looking forward to the future as far as
stacking multiple layers of tunnels, I think the only consistent thing
to do is have the inner ID increment and all of the tunnel headers
stay fixed - it is hard to justify why the first tunnel header should
increment but not the second one. And finally, as a nice bonus, this
is what the GRO code has been expecting already so you won't cause any
performance regressions with existing systems.

^ permalink raw reply

* Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption
From: Eric Dumazet @ 2016-03-24  1:39 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Subash Abhinov Kasiviswanathan, 'Steffen Klassert',
	netdev, jeromes
In-Reply-To: <20160324004514.GA3110@gondor.apana.org.au>

On Thu, 2016-03-24 at 08:45 +0800, Herbert Xu wrote:
> On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote:
> >
> > OK, but before calling netif_rx() are we properly testing dev->flags
> > IFF_UP status ?
> > 
> > Otherwise, we still allow packets being queued after flush_backlog() had
> > been called.
> 
> That's the first thing enqueue_to_backlog tests.
> 
> Cheers,

Seems to be very recent stuff ( commit
e9e4dd3267d0c5234c5c0f47440456b10875dec9 in linux-4.2)

In the old days the test was done in callers, since in most cases NIC
drivers do not need it.

Lets make sure this was backported to all stable trees.

And then we probably can cleanup some callers as well.

^ permalink raw reply

* Re: [RFC PATCH 1/9] ipv4/GRO: Allow multiple frames to use the same IP ID
From: Jesse Gross @ 2016-03-24  1:43 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Edward Cree, Linux Kernel Network Developers, David Miller,
	Alexander Duyck, Tom Herbert
In-Reply-To: <20160318232444.14955.38133.stgit@localhost.localdomain>

On Fri, Mar 18, 2016 at 4:24 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
> In RFC 6864 it is stated that we can essentially ignore the IPv4 ID field
> if we have not and will not use fragmentation.  Such a frame is defined
> as having the DF flag set to 1, and the MF and frag_offset as 0.  Currently
> for GRO we were requiring that the inner header always have an increasing
> IPv4 ID, but we are ignoring the outer value.
>
> This patch is a first step in trying to reverse some of that.  Specifically
> what this patch does is allow us to coalesce frames that have a static IPv4
> ID value.  So for example if we had a series of frames where the DF flag
> was set we would allow the same IPv4 ID value to be used for all the frames
> belonging to that flow.  This would become the standard behavior for TCP so
> it would support either a fixed IPv4 ID value, or one in which the value
> increments.
>
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

One thing that is a bit odd is that the TSO output procedure stays the
same. So that means that if we get a stream of packets with the DF bit
set and a constant IP ID, aggregate them with GRO, and the retransmit
with GSO/TSO then we'll get packets with IDs that increment for each
burst and then start back again to the original value. I guess it
doesn't matter in practice since the IDs are supposed to be ignored
but it does seem a little strange - especially because the new packets
will now be violating the rules of the same GRO implementation that
produced them.

^ permalink raw reply

* Re: [PATCH net-next] hv_netvsc: Fix the order of num_sc_offered decrement
From: David Miller @ 2016-03-24  1:53 UTC (permalink / raw)
  To: haiyangz; +Cc: olaf, netdev, driverdev-devel, linux-kernel
In-Reply-To: <1458770088-923-1-git-send-email-haiyangz@microsoft.com>

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Wed, 23 Mar 2016 14:54:48 -0700

> Reorder the code in netvsc_sc_open(), so num_sc_offered is only decremented
> after vmbus_open() is called. This avoid pontential race of removing device
> before all channels are setup.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption
From: David Miller @ 2016-03-24  2:08 UTC (permalink / raw)
  To: eric.dumazet; +Cc: herbert, subashab, steffen.klassert, netdev, jeromes
In-Reply-To: <1458783597.10868.60.camel@edumazet-glaptop3.roam.corp.google.com>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 23 Mar 2016 18:39:57 -0700

> On Thu, 2016-03-24 at 08:45 +0800, Herbert Xu wrote:
>> On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote:
>> >
>> > OK, but before calling netif_rx() are we properly testing dev->flags
>> > IFF_UP status ?
>> > 
>> > Otherwise, we still allow packets being queued after flush_backlog() had
>> > been called.
>> 
>> That's the first thing enqueue_to_backlog tests.
>> 
>> Cheers,
> 
> Seems to be very recent stuff ( commit
> e9e4dd3267d0c5234c5c0f47440456b10875dec9 in linux-4.2)
> 
> In the old days the test was done in callers, since in most cases NIC
> drivers do not need it.
> 
> Lets make sure this was backported to all stable trees.
> 
> And then we probably can cleanup some callers as well.

Anyways this patch needs to be redone because it is corrupted by the
submitter's email client.

I'll queue it up and make sure e9e4dd3267d0c5234c5c0f47440456b10875dec9
ends up in -stable where needed.

Thanks.

^ permalink raw reply

* Re: [PATCH] net: ping: make ping_v6_sendmsg static
From: David Miller @ 2016-03-24  2:10 UTC (permalink / raw)
  To: yanhaishuang
  Cc: sergei.shtylyov, jmorris, kaber, kuznet, yoshfuji, netdev,
	linux-kernel
In-Reply-To: <etPan.56f3489d.47f24d7.66d@yanhaishuangs-MacBook-Pro.local>

From: 严海双 <yanhaishuang@cmss.chinamobile.com>
Date: Thu, 24 Mar 2016 09:53:33 +0800

> The two changes is dependent, because "pingv6_prot" and
> “pingv6_protosw” must be moved to the behind also, to avoid having
> to declare static “ping_v6_sendmsg” prototypes first.

Agreed, and applied, thank you.

^ permalink raw reply

* Re: [RFC PATCH 1/9] ipv4/GRO: Allow multiple frames to use the same IP ID
From: Alexander Duyck @ 2016-03-24  2:21 UTC (permalink / raw)
  To: Jesse Gross
  Cc: Alexander Duyck, Edward Cree, Linux Kernel Network Developers,
	David Miller, Tom Herbert
In-Reply-To: <CAEh+42ir0ich=MyEtr7NkKiUBhE7Md_AF8YzQ2bCv=cy=5ziFg@mail.gmail.com>

On Wed, Mar 23, 2016 at 6:43 PM, Jesse Gross <jesse@kernel.org> wrote:
> On Fri, Mar 18, 2016 at 4:24 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>> In RFC 6864 it is stated that we can essentially ignore the IPv4 ID field
>> if we have not and will not use fragmentation.  Such a frame is defined
>> as having the DF flag set to 1, and the MF and frag_offset as 0.  Currently
>> for GRO we were requiring that the inner header always have an increasing
>> IPv4 ID, but we are ignoring the outer value.
>>
>> This patch is a first step in trying to reverse some of that.  Specifically
>> what this patch does is allow us to coalesce frames that have a static IPv4
>> ID value.  So for example if we had a series of frames where the DF flag
>> was set we would allow the same IPv4 ID value to be used for all the frames
>> belonging to that flow.  This would become the standard behavior for TCP so
>> it would support either a fixed IPv4 ID value, or one in which the value
>> increments.
>>
>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>
> One thing that is a bit odd is that the TSO output procedure stays the
> same. So that means that if we get a stream of packets with the DF bit
> set and a constant IP ID, aggregate them with GRO, and the retransmit
> with GSO/TSO then we'll get packets with IDs that increment for each
> burst and then start back again to the original value. I guess it
> doesn't matter in practice since the IDs are supposed to be ignored
> but it does seem a little strange - especially because the new packets
> will now be violating the rules of the same GRO implementation that
> produced them.

Yes and no.  The rule for GRO with this patch is that the IP ID has to
be either incrementing or if DF is set it has the option to be a fixed
value for a given grouping of packets.  In that regard either GSO
partial or standard GSO are still both reversible via GRO so that you
can aggregate to get back to the original frame (ignoring the IP ID)
that GSO segmented.  The bit I am still trying to work out is what to
do about the case where we GRO 2 frames out of one GSO segment.  I
wonder if I should just totally ignore the IP ID value since it ends
up creating an artificial boundary between the two frames if they are
segmented using the incrementing GSO versus the fixed IP ID GSO.

- Alex

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox