[PATCH net-next 0/2] vxlan: Don't use UDP socket for transmit

[PATCH net-next 0/2] vxlan: Don't use UDP socket for transmit

[Tom Herbert]

UDP socket is not pertinent to transmit for UDP tunnels, checksum
enablement can be done with a socket. This patch set eliminates
reference to a socket in udp_tunnel_xmit functions and in VXLAN
trnasmit.

Also, make GBP, RCO, can CSUM6_RX flags visible to receive socket
and only match these for shareable socket.
Tom Herbert (2):
  udp: Do not require sock in udp_tunnel_xmit_skb
  vxlan: Eliminate dependency on UDP socket in transmit path

 drivers/net/vxlan.c           | 66 +++++++++++++++++++++----------------------
 include/net/udp_tunnel.h      | 16 +++++------
 include/net/vxlan.h           | 13 +++++----
 net/ipv4/udp_tunnel.c         | 12 ++++----
 net/ipv6/ip6_udp_tunnel.c     | 12 ++++----
 net/openvswitch/vport-vxlan.c |  6 ++--
 6 files changed, 62 insertions(+), 63 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Tom Herbert]

The UDP tunnel transmit functions udp_tunnel_xmit_skb and
udp_tunnel6_xmit_skb include a socket argument. The socket being
passed to the functions (from VXLAN) is a UDP created for receive
side. The only thing that the socket is used for in the transmit
functions is to get the setting for checksum (enabled or zero).
This patch removes the argument and and adds a nocheck argument
for checksum setting. This eliminates the unnecessary dependency
on a UDP socket for UDP tunnel transmit.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 drivers/net/vxlan.c       | 10 ++++++----
 include/net/udp_tunnel.h  | 16 ++++++++--------
 net/ipv4/udp_tunnel.c     | 12 ++++++------
 net/ipv6/ip6_udp_tunnel.c | 12 ++++++------
 4 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 6b6b456..4fb4205 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1763,8 +1763,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
 
 	skb_set_inner_protocol(skb, htons(ETH_P_TEB));
 
-	udp_tunnel6_xmit_skb(vs->sock, dst, skb, dev, saddr, daddr, prio,
-			     ttl, src_port, dst_port);
+	udp_tunnel6_xmit_skb(dst, skb, dev, saddr, daddr, prio,
+			     ttl, src_port, dst_port,
+			     udp_get_no_check6_tx(vs->sock->sk));
 	return 0;
 err:
 	dst_release(dst);
@@ -1842,8 +1843,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
 
 	skb_set_inner_protocol(skb, htons(ETH_P_TEB));
 
-	return udp_tunnel_xmit_skb(vs->sock, rt, skb, src, dst, tos,
-				   ttl, df, src_port, dst_port, xnet);
+	return udp_tunnel_xmit_skb(rt, skb, src, dst, tos,
+				   ttl, df, src_port, dst_port, xnet,
+				   vs->sock->sk->sk_no_check_tx);
 }
 EXPORT_SYMBOL_GPL(vxlan_xmit_skb);
 
diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index 2a50a70..1a20d33 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -77,17 +77,17 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
 			   struct udp_tunnel_sock_cfg *sock_cfg);
 
 /* Transmit the skb using UDP encapsulation. */
-int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
-			struct sk_buff *skb, __be32 src, __be32 dst,
-			__u8 tos, __u8 ttl, __be16 df, __be16 src_port,
-			__be16 dst_port, bool xnet);
+int udp_tunnel_xmit_skb(struct rtable *rt, struct sk_buff *skb,
+			__be32 src, __be32 dst, __u8 tos, __u8 ttl,
+			__be16 df, __be16 src_port, __be16 dst_port,
+			bool xnet, bool nocheck);
 
 #if IS_ENABLED(CONFIG_IPV6)
-int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
-			 struct sk_buff *skb, struct net_device *dev,
-			 struct in6_addr *saddr, struct in6_addr *daddr,
+int udp_tunnel6_xmit_skb(struct dst_entry *dst, struct sk_buff *skb,
+			 struct net_device *dev, struct in6_addr *saddr,
+			 struct in6_addr *daddr,
 			 __u8 prio, __u8 ttl, __be16 src_port,
-			 __be16 dst_port);
+			 __be16 dst_port, bool nocheck);
 #endif
 
 void udp_tunnel_sock_release(struct socket *sock);
diff --git a/net/ipv4/udp_tunnel.c b/net/ipv4/udp_tunnel.c
index 9996e63..c83b354 100644
--- a/net/ipv4/udp_tunnel.c
+++ b/net/ipv4/udp_tunnel.c
@@ -75,10 +75,10 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
 }
 EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
 
-int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
-			struct sk_buff *skb, __be32 src, __be32 dst,
-			__u8 tos, __u8 ttl, __be16 df, __be16 src_port,
-			__be16 dst_port, bool xnet)
+int udp_tunnel_xmit_skb(struct rtable *rt, struct sk_buff *skb,
+			__be32 src, __be32 dst, __u8 tos, __u8 ttl,
+			__be16 df, __be16 src_port, __be16 dst_port,
+			bool xnet, bool nocheck)
 {
 	struct udphdr *uh;
 
@@ -90,9 +90,9 @@ int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
 	uh->source = src_port;
 	uh->len = htons(skb->len);
 
-	udp_set_csum(sock->sk->sk_no_check_tx, skb, src, dst, skb->len);
+	udp_set_csum(nocheck, skb, src, dst, skb->len);
 
-	return iptunnel_xmit(sock->sk, rt, skb, src, dst, IPPROTO_UDP,
+	return iptunnel_xmit(skb->sk, rt, skb, src, dst, IPPROTO_UDP,
 			     tos, ttl, df, xnet);
 }
 EXPORT_SYMBOL_GPL(udp_tunnel_xmit_skb);
diff --git a/net/ipv6/ip6_udp_tunnel.c b/net/ipv6/ip6_udp_tunnel.c
index 8db6c98..32d9b26 100644
--- a/net/ipv6/ip6_udp_tunnel.c
+++ b/net/ipv6/ip6_udp_tunnel.c
@@ -62,14 +62,14 @@ error:
 }
 EXPORT_SYMBOL_GPL(udp_sock_create6);
 
-int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
-			 struct sk_buff *skb, struct net_device *dev,
-			 struct in6_addr *saddr, struct in6_addr *daddr,
-			 __u8 prio, __u8 ttl, __be16 src_port, __be16 dst_port)
+int udp_tunnel6_xmit_skb(struct dst_entry *dst, struct sk_buff *skb,
+			 struct net_device *dev, struct in6_addr *saddr,
+			 struct in6_addr *daddr,
+			 __u8 prio, __u8 ttl, __be16 src_port,
+			 __be16 dst_port, bool nocheck)
 {
 	struct udphdr *uh;
 	struct ipv6hdr *ip6h;
-	struct sock *sk = sock->sk;
 
 	__skb_push(skb, sizeof(*uh));
 	skb_reset_transport_header(skb);
@@ -85,7 +85,7 @@ int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
 			    | IPSKB_REROUTED);
 	skb_dst_set(skb, dst);
 
-	udp6_set_csum(udp_get_no_check6_tx(sk), skb, saddr, daddr, skb->len);
+	udp6_set_csum(nocheck, skb, saddr, daddr, skb->len);
 
 	__skb_push(skb, sizeof(*ip6h));
 	skb_reset_network_header(skb);
-- 
2.2.0.rc0.207.ga3a616c

[PATCH net-next 2/2] vxlan: Eliminate dependency on UDP socket in transmit path

[Tom Herbert]

In the vxlan transmit path there is no need to reference the socket
for a tunnel which is needed for the receive side. We do, however,
need the vxlan_dev flags. This patch eliminate references
to the socket in the transmit path, and changes VXLAN_F_UNSHAREABLE
to be VXLAN_F_RCV_FLAGS. This mask is used to store the flags
applicable to receive (GBP, CSUM6_RX, and REMCSUM_RX) in the
vxlan_sock flags.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 drivers/net/vxlan.c           | 60 ++++++++++++++++++++-----------------------
 include/net/vxlan.h           | 13 ++++++----
 net/openvswitch/vport-vxlan.c |  6 ++---
 3 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 4fb4205..fb7805b 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -270,12 +270,13 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
 					  __be16 port, u32 flags)
 {
 	struct vxlan_sock *vs;
-	u32 match_flags = flags & VXLAN_F_UNSHAREABLE;
+
+	flags &= VXLAN_F_RCV_FLAGS;
 
 	hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) {
 		if (inet_sk(vs->sock->sk)->inet_sport == port &&
 		    inet_sk(vs->sock->sk)->sk.sk_family == family &&
-		    (vs->flags & VXLAN_F_UNSHAREABLE) == match_flags)
+		    vs->flags == flags)
 			return vs;
 	}
 	return NULL;
@@ -1668,7 +1669,7 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
 	return false;
 }
 
-static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_sock *vs,
+static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
 				struct vxlan_metadata *md)
 {
 	struct vxlanhdr_gbp *gbp;
@@ -1686,21 +1687,20 @@ static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_sock *vs,
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
-static int vxlan6_xmit_skb(struct vxlan_sock *vs,
-			   struct dst_entry *dst, struct sk_buff *skb,
+static int vxlan6_xmit_skb(struct dst_entry *dst, struct sk_buff *skb,
 			   struct net_device *dev, struct in6_addr *saddr,
 			   struct in6_addr *daddr, __u8 prio, __u8 ttl,
 			   __be16 src_port, __be16 dst_port,
-			   struct vxlan_metadata *md, bool xnet)
+			   struct vxlan_metadata *md, bool xnet, u32 vxflags)
 {
 	struct vxlanhdr *vxh;
 	int min_headroom;
 	int err;
-	bool udp_sum = !udp_get_no_check6_tx(vs->sock->sk);
+	bool udp_sum = !(vxflags & VXLAN_F_UDP_ZERO_CSUM6_TX);
 	int type = udp_sum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
 	u16 hdrlen = sizeof(struct vxlanhdr);
 
-	if ((vs->flags & VXLAN_F_REMCSUM_TX) &&
+	if ((vxflags & VXLAN_F_REMCSUM_TX) &&
 	    skb->ip_summed == CHECKSUM_PARTIAL) {
 		int csum_start = skb_checksum_start_offset(skb);
 
@@ -1758,14 +1758,14 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
 		}
 	}
 
-	if (vs->flags & VXLAN_F_GBP)
-		vxlan_build_gbp_hdr(vxh, vs, md);
+	if (vxflags & VXLAN_F_GBP)
+		vxlan_build_gbp_hdr(vxh, vxflags, md);
 
 	skb_set_inner_protocol(skb, htons(ETH_P_TEB));
 
 	udp_tunnel6_xmit_skb(dst, skb, dev, saddr, daddr, prio,
 			     ttl, src_port, dst_port,
-			     udp_get_no_check6_tx(vs->sock->sk));
+			     !!(vxflags & VXLAN_F_UDP_ZERO_CSUM6_TX));
 	return 0;
 err:
 	dst_release(dst);
@@ -1773,20 +1773,19 @@ err:
 }
 #endif
 
-int vxlan_xmit_skb(struct vxlan_sock *vs,
-		   struct rtable *rt, struct sk_buff *skb,
+int vxlan_xmit_skb(struct rtable *rt, struct sk_buff *skb,
 		   __be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
 		   __be16 src_port, __be16 dst_port,
-		   struct vxlan_metadata *md, bool xnet)
+		   struct vxlan_metadata *md, bool xnet, u32 vxflags)
 {
 	struct vxlanhdr *vxh;
 	int min_headroom;
 	int err;
-	bool udp_sum = !vs->sock->sk->sk_no_check_tx;
+	bool udp_sum = !!(vxflags & VXLAN_F_UDP_CSUM);
 	int type = udp_sum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
 	u16 hdrlen = sizeof(struct vxlanhdr);
 
-	if ((vs->flags & VXLAN_F_REMCSUM_TX) &&
+	if ((vxflags & VXLAN_F_REMCSUM_TX) &&
 	    skb->ip_summed == CHECKSUM_PARTIAL) {
 		int csum_start = skb_checksum_start_offset(skb);
 
@@ -1838,14 +1837,14 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
 		}
 	}
 
-	if (vs->flags & VXLAN_F_GBP)
-		vxlan_build_gbp_hdr(vxh, vs, md);
+	if (vxflags & VXLAN_F_GBP)
+		vxlan_build_gbp_hdr(vxh, vxflags, md);
 
 	skb_set_inner_protocol(skb, htons(ETH_P_TEB));
 
 	return udp_tunnel_xmit_skb(rt, skb, src, dst, tos,
 				   ttl, df, src_port, dst_port, xnet,
-				   vs->sock->sk->sk_no_check_tx);
+				   !(vxflags & VXLAN_F_UDP_CSUM));
 }
 EXPORT_SYMBOL_GPL(vxlan_xmit_skb);
 
@@ -1977,10 +1976,11 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		md.vni = htonl(vni << 8);
 		md.gbp = skb->mark;
 
-		err = vxlan_xmit_skb(vxlan->vn_sock, rt, skb,
-				     fl4.saddr, dst->sin.sin_addr.s_addr,
-				     tos, ttl, df, src_port, dst_port, &md,
-				     !net_eq(vxlan->net, dev_net(vxlan->dev)));
+		err = vxlan_xmit_skb(rt, skb, fl4.saddr,
+				     dst->sin.sin_addr.s_addr, tos, ttl, df,
+				     src_port, dst_port, &md,
+				     !net_eq(vxlan->net, dev_net(vxlan->dev)),
+				     vxlan->flags);
 		if (err < 0) {
 			/* skb is already freed. */
 			skb = NULL;
@@ -2036,10 +2036,10 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		md.vni = htonl(vni << 8);
 		md.gbp = skb->mark;
 
-		err = vxlan6_xmit_skb(vxlan->vn_sock, ndst, skb,
-				      dev, &fl6.saddr, &fl6.daddr, 0, ttl,
-				      src_port, dst_port, &md,
-				      !net_eq(vxlan->net, dev_net(vxlan->dev)));
+		err = vxlan6_xmit_skb(ndst, skb, dev, &fl6.saddr, &fl6.daddr,
+				      0, ttl, src_port, dst_port, &md,
+				      !net_eq(vxlan->net, dev_net(vxlan->dev)),
+				      vxlan->flags);
 #endif
 	}
 
@@ -2511,15 +2511,11 @@ static struct socket *vxlan_create_sock(struct net *net, bool ipv6,
 
 	if (ipv6) {
 		udp_conf.family = AF_INET6;
-		udp_conf.use_udp6_tx_checksums =
-		    !(flags & VXLAN_F_UDP_ZERO_CSUM6_TX);
 		udp_conf.use_udp6_rx_checksums =
 		    !(flags & VXLAN_F_UDP_ZERO_CSUM6_RX);
 	} else {
 		udp_conf.family = AF_INET;
 		udp_conf.local_ip.s_addr = INADDR_ANY;
-		udp_conf.use_udp_checksums =
-		    !!(flags & VXLAN_F_UDP_CSUM);
 	}
 
 	udp_conf.local_udp_port = port;
@@ -2563,7 +2559,7 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port,
 	atomic_set(&vs->refcnt, 1);
 	vs->rcv = rcv;
 	vs->data = data;
-	vs->flags = flags;
+	vs->flags = (flags & VXLAN_F_RCV_FLAGS);
 
 	/* Initialize the vxlan udp offloads structure */
 	vs->udp_offloads.port = port;
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 7be8c34..2927d62 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -129,8 +129,12 @@ struct vxlan_sock {
 #define VXLAN_F_REMCSUM_RX		0x400
 #define VXLAN_F_GBP			0x800
 
-/* These flags must match in order for a socket to be shareable */
-#define VXLAN_F_UNSHAREABLE		VXLAN_F_GBP
+/* Flags that are used in the receive patch. These flags must match in
+ * order for a socket to be shareable
+ */
+#define VXLAN_F_RCV_FLAGS		(VXLAN_F_GBP |			\
+					 VXLAN_F_UDP_ZERO_CSUM6_RX |	\
+					 VXLAN_F_REMCSUM_RX)
 
 struct vxlan_sock *vxlan_sock_add(struct net *net, __be16 port,
 				  vxlan_rcv_t *rcv, void *data,
@@ -138,11 +142,10 @@ struct vxlan_sock *vxlan_sock_add(struct net *net, __be16 port,
 
 void vxlan_sock_release(struct vxlan_sock *vs);
 
-int vxlan_xmit_skb(struct vxlan_sock *vs,
-		   struct rtable *rt, struct sk_buff *skb,
+int vxlan_xmit_skb(struct rtable *rt, struct sk_buff *skb,
 		   __be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
 		   __be16 src_port, __be16 dst_port, struct vxlan_metadata *md,
-		   bool xnet);
+		   bool xnet, u32 vxflags);
 
 static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
 						     netdev_features_t features)
diff --git a/net/openvswitch/vport-vxlan.c b/net/openvswitch/vport-vxlan.c
index 8a2d54c..3cc983b 100644
--- a/net/openvswitch/vport-vxlan.c
+++ b/net/openvswitch/vport-vxlan.c
@@ -252,12 +252,10 @@ static int vxlan_tnl_send(struct vport *vport, struct sk_buff *skb)
 	md.vni = htonl(be64_to_cpu(tun_key->tun_id) << 8);
 	md.gbp = vxlan_ext_gbp(skb);
 
-	err = vxlan_xmit_skb(vxlan_port->vs, rt, skb,
-			     fl.saddr, tun_key->ipv4_dst,
+	err = vxlan_xmit_skb(rt, skb, fl.saddr, tun_key->ipv4_dst,
 			     tun_key->ipv4_tos, tun_key->ipv4_ttl, df,
 			     src_port, dst_port,
-			     &md,
-			     false);
+			     &md, false, vxlan_port->exts);
 	if (err < 0)
 		ip_rt_put(rt);
 	return err;
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Jesse Gross]

On Sat, Jan 17, 2015 at 10:18 AM, Tom Herbert <therbert@google.com> wrote:
> The UDP tunnel transmit functions udp_tunnel_xmit_skb and
> udp_tunnel6_xmit_skb include a socket argument. The socket being
> passed to the functions (from VXLAN) is a UDP created for receive
> side. The only thing that the socket is used for in the transmit
> functions is to get the setting for checksum (enabled or zero).
> This patch removes the argument and and adds a nocheck argument
> for checksum setting. This eliminates the unnecessary dependency
> on a UDP socket for UDP tunnel transmit.
>
> Signed-off-by: Tom Herbert <therbert@google.com>

I think you need to update Geneve as well:

net/ipv4/geneve.c:139:38: warning: incorrect type in argument 1
(different base types)
net/ipv4/geneve.c:139:38:    expected struct rtable *rt
net/ipv4/geneve.c:139:38:    got struct socket *sock
net/ipv4/geneve.c:139:46: warning: incorrect type in argument 2
(different base types)
net/ipv4/geneve.c:139:46:    expected struct sk_buff *skb
net/ipv4/geneve.c:139:46:    got struct rtable *rt
net/ipv4/geneve.c:139:50: warning: incorrect type in argument 3
(different base types)
net/ipv4/geneve.c:139:50:    expected restricted __be32 [usertype] src
net/ipv4/geneve.c:139:50:    got struct sk_buff *[assigned] skb
net/ipv4/geneve.c:139:60: warning: incorrect type in argument 5
(different base types)
net/ipv4/geneve.c:139:60:    expected unsigned char [unsigned] [usertype] tos
net/ipv4/geneve.c:139:60:    got restricted __be32 [usertype] dst
net/ipv4/geneve.c:140:41: warning: incorrect type in argument 7
(different base types)
net/ipv4/geneve.c:140:41:    expected restricted __be16 [usertype] df
net/ipv4/geneve.c:140:41:    got unsigned char [unsigned] [usertype] ttl
net/ipv4/geneve.c:140:60: warning: incorrect type in argument 10
(different base types)
net/ipv4/geneve.c:140:60:    expected bool [unsigned] [usertype] xnet
net/ipv4/geneve.c:140:60:    got restricted __be16 [usertype] dst_port

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Or Gerlitz]

On Sat, Jan 17, 2015 at 8:18 PM, Tom Herbert <therbert@google.com> wrote:
> The UDP tunnel transmit functions udp_tunnel_xmit_skb and
> udp_tunnel6_xmit_skb include a socket argument. The socket being
> passed to the functions (from VXLAN) is a UDP created for receive
> side. The only thing that the socket is used for in the transmit
> functions is to get the setting for checksum (enabled or zero).

Tom, just to clarify - re the sockets usage in the transmit side,
somewhere bind or alike is done on them such that we have multiple
source UDP ports for given host VXLAN traffic. Here for example the
sender host is 192.168.31.17 and two ports are seen here 54206 and
50795.

Just wanted to make sure this series doesn't change that, since if
this is the case, we introduce here a regression w.r.t RSS hash
spreading from the outer UDP header data at the receiver side (which
is the right thing to do, per your LKS session...)

IP 192.168.31.18.45367 > 192.168.31.17.4789: UDP, length 62
IP 192.168.31.18.45515 > 192.168.31.17.4789: UDP, length 62
IP 192.168.31.17.54206 > 192.168.31.18.4789: UDP, length 26814
IP 192.168.31.18.45367 > 192.168.31.17.4789: UDP, length 62
IP 192.168.31.18.45515 > 192.168.31.17.4789: UDP, length 62
IP 192.168.31.17.50795 > 192.168.31.18.4789: UDP, length 25498
IP 192.168.31.17.50795 > 192.168.31.18.4789: UDP, length 64922
IP 192.168.31.17.50795 > 192.168.31.18.4789: UDP, length 64922
IP 192.168.31.18.45367 > 192.168.31.17.4789: UDP, length 62
IP 192.168.31.17.54206 > 192.168.31.18.4789: UDP, length 38170

> This patch removes the argument and and adds a nocheck argument
> for checksum setting. This eliminates the unnecessary dependency
> on a UDP socket for UDP tunnel transmit.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
>  drivers/net/vxlan.c       | 10 ++++++----
>  include/net/udp_tunnel.h  | 16 ++++++++--------
>  net/ipv4/udp_tunnel.c     | 12 ++++++------
>  net/ipv6/ip6_udp_tunnel.c | 12 ++++++------
>  4 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 6b6b456..4fb4205 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1763,8 +1763,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
>
>         skb_set_inner_protocol(skb, htons(ETH_P_TEB));
>
> -       udp_tunnel6_xmit_skb(vs->sock, dst, skb, dev, saddr, daddr, prio,
> -                            ttl, src_port, dst_port);
> +       udp_tunnel6_xmit_skb(dst, skb, dev, saddr, daddr, prio,
> +                            ttl, src_port, dst_port,
> +                            udp_get_no_check6_tx(vs->sock->sk));
>         return 0;
>  err:
>         dst_release(dst);
> @@ -1842,8 +1843,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
>
>         skb_set_inner_protocol(skb, htons(ETH_P_TEB));
>
> -       return udp_tunnel_xmit_skb(vs->sock, rt, skb, src, dst, tos,
> -                                  ttl, df, src_port, dst_port, xnet);
> +       return udp_tunnel_xmit_skb(rt, skb, src, dst, tos,
> +                                  ttl, df, src_port, dst_port, xnet,
> +                                  vs->sock->sk->sk_no_check_tx);
>  }
>  EXPORT_SYMBOL_GPL(vxlan_xmit_skb);
>
> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> index 2a50a70..1a20d33 100644
> --- a/include/net/udp_tunnel.h
> +++ b/include/net/udp_tunnel.h
> @@ -77,17 +77,17 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>                            struct udp_tunnel_sock_cfg *sock_cfg);
>
>  /* Transmit the skb using UDP encapsulation. */
> -int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
> -                       struct sk_buff *skb, __be32 src, __be32 dst,
> -                       __u8 tos, __u8 ttl, __be16 df, __be16 src_port,
> -                       __be16 dst_port, bool xnet);
> +int udp_tunnel_xmit_skb(struct rtable *rt, struct sk_buff *skb,
> +                       __be32 src, __be32 dst, __u8 tos, __u8 ttl,
> +                       __be16 df, __be16 src_port, __be16 dst_port,
> +                       bool xnet, bool nocheck);
>
>  #if IS_ENABLED(CONFIG_IPV6)
> -int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
> -                        struct sk_buff *skb, struct net_device *dev,
> -                        struct in6_addr *saddr, struct in6_addr *daddr,
> +int udp_tunnel6_xmit_skb(struct dst_entry *dst, struct sk_buff *skb,
> +                        struct net_device *dev, struct in6_addr *saddr,
> +                        struct in6_addr *daddr,
>                          __u8 prio, __u8 ttl, __be16 src_port,
> -                        __be16 dst_port);
> +                        __be16 dst_port, bool nocheck);
>  #endif
>
>  void udp_tunnel_sock_release(struct socket *sock);
> diff --git a/net/ipv4/udp_tunnel.c b/net/ipv4/udp_tunnel.c
> index 9996e63..c83b354 100644
> --- a/net/ipv4/udp_tunnel.c
> +++ b/net/ipv4/udp_tunnel.c
> @@ -75,10 +75,10 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>  }
>  EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
>
> -int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
> -                       struct sk_buff *skb, __be32 src, __be32 dst,
> -                       __u8 tos, __u8 ttl, __be16 df, __be16 src_port,
> -                       __be16 dst_port, bool xnet)
> +int udp_tunnel_xmit_skb(struct rtable *rt, struct sk_buff *skb,
> +                       __be32 src, __be32 dst, __u8 tos, __u8 ttl,
> +                       __be16 df, __be16 src_port, __be16 dst_port,
> +                       bool xnet, bool nocheck)
>  {
>         struct udphdr *uh;
>
> @@ -90,9 +90,9 @@ int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
>         uh->source = src_port;
>         uh->len = htons(skb->len);
>
> -       udp_set_csum(sock->sk->sk_no_check_tx, skb, src, dst, skb->len);
> +       udp_set_csum(nocheck, skb, src, dst, skb->len);
>
> -       return iptunnel_xmit(sock->sk, rt, skb, src, dst, IPPROTO_UDP,
> +       return iptunnel_xmit(skb->sk, rt, skb, src, dst, IPPROTO_UDP,
>                              tos, ttl, df, xnet);
>  }
>  EXPORT_SYMBOL_GPL(udp_tunnel_xmit_skb);
> diff --git a/net/ipv6/ip6_udp_tunnel.c b/net/ipv6/ip6_udp_tunnel.c
> index 8db6c98..32d9b26 100644
> --- a/net/ipv6/ip6_udp_tunnel.c
> +++ b/net/ipv6/ip6_udp_tunnel.c
> @@ -62,14 +62,14 @@ error:
>  }
>  EXPORT_SYMBOL_GPL(udp_sock_create6);
>
> -int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
> -                        struct sk_buff *skb, struct net_device *dev,
> -                        struct in6_addr *saddr, struct in6_addr *daddr,
> -                        __u8 prio, __u8 ttl, __be16 src_port, __be16 dst_port)
> +int udp_tunnel6_xmit_skb(struct dst_entry *dst, struct sk_buff *skb,
> +                        struct net_device *dev, struct in6_addr *saddr,
> +                        struct in6_addr *daddr,
> +                        __u8 prio, __u8 ttl, __be16 src_port,
> +                        __be16 dst_port, bool nocheck)
>  {
>         struct udphdr *uh;
>         struct ipv6hdr *ip6h;
> -       struct sock *sk = sock->sk;
>
>         __skb_push(skb, sizeof(*uh));
>         skb_reset_transport_header(skb);
> @@ -85,7 +85,7 @@ int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
>                             | IPSKB_REROUTED);
>         skb_dst_set(skb, dst);
>
> -       udp6_set_csum(udp_get_no_check6_tx(sk), skb, saddr, daddr, skb->len);
> +       udp6_set_csum(nocheck, skb, saddr, daddr, skb->len);
>
>         __skb_push(skb, sizeof(*ip6h));
>         skb_reset_network_header(skb);
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 2/2] vxlan: Eliminate dependency on UDP socket in transmit path

[Thomas Graf]

On 01/17/15 at 10:18am, Tom Herbert wrote:
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 7be8c34..2927d62 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -129,8 +129,12 @@ struct vxlan_sock {
>  #define VXLAN_F_REMCSUM_RX		0x400
>  #define VXLAN_F_GBP			0x800
>  
> -/* These flags must match in order for a socket to be shareable */
> -#define VXLAN_F_UNSHAREABLE		VXLAN_F_GBP
> +/* Flags that are used in the receive patch. These flags must match in
                                          ^^^^^



> + * order for a socket to be shareable
> + */
> +#define VXLAN_F_RCV_FLAGS		(VXLAN_F_GBP |			\
> +					 VXLAN_F_UDP_ZERO_CSUM6_RX |	\
> +					 VXLAN_F_REMCSUM_RX)

I'm fine with this. It is slightly odd that we will be transmitting
RCO and other extensions on UDP ports which cannot accept the same
frames. I assume you have specific use cases for this scenario.

Re: [PATCH net-next 2/2] vxlan: Eliminate dependency on UDP socket in transmit path

[Tom Herbert]

On Mon, Jan 19, 2015 at 12:59 AM, Thomas Graf <tgraf@suug.ch> wrote:
> On 01/17/15 at 10:18am, Tom Herbert wrote:
>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>> index 7be8c34..2927d62 100644
>> --- a/include/net/vxlan.h
>> +++ b/include/net/vxlan.h
>> @@ -129,8 +129,12 @@ struct vxlan_sock {
>>  #define VXLAN_F_REMCSUM_RX           0x400
>>  #define VXLAN_F_GBP                  0x800
>>
>> -/* These flags must match in order for a socket to be shareable */
>> -#define VXLAN_F_UNSHAREABLE          VXLAN_F_GBP
>> +/* Flags that are used in the receive patch. These flags must match in
>                                           ^^^^^
>
>
>
>> + * order for a socket to be shareable
>> + */
>> +#define VXLAN_F_RCV_FLAGS            (VXLAN_F_GBP |                  \
>> +                                      VXLAN_F_UDP_ZERO_CSUM6_RX |    \
>> +                                      VXLAN_F_REMCSUM_RX)
>
> I'm fine with this. It is slightly odd that we will be transmitting
> RCO and other extensions on UDP ports which cannot accept the same
> frames. I assume you have specific use cases for this scenario.

I didn't see any reason to preclude that, if it needs to be symmetric
in that case it can be forced at the configuration. Being able to
receive RCO but not have to send it to certain peers is important use
case. You may want to consider this also for GBP if there are cases
where we accept GBP from different peers, but only send it to certain
ones.

Tom

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Tom Herbert]

On Sun, Jan 18, 2015 at 2:43 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Sat, Jan 17, 2015 at 8:18 PM, Tom Herbert <therbert@google.com> wrote:
>> The UDP tunnel transmit functions udp_tunnel_xmit_skb and
>> udp_tunnel6_xmit_skb include a socket argument. The socket being
>> passed to the functions (from VXLAN) is a UDP created for receive
>> side. The only thing that the socket is used for in the transmit
>> functions is to get the setting for checksum (enabled or zero).
>
> Tom, just to clarify - re the sockets usage in the transmit side,
> somewhere bind or alike is done on them such that we have multiple
> source UDP ports for given host VXLAN traffic. Here for example the
> sender host is 192.168.31.17 and two ports are seen here 54206 and
> 50795.
>
> Just wanted to make sure this series doesn't change that, since if
> this is the case, we introduce here a regression w.r.t RSS hash
> spreading from the outer UDP header data at the receiver side (which
> is the right thing to do, per your LKS session...)
>
Hi Or,

Using or not using a socket on transmit should have no bearing to the
receive side. RSS works based on the hash of the UDP 5-tuple which
should include a source port set to a value for the inner flow. Since
the UDP socket is unconnected it should have no bearing on RFS or XPS
either.

By the way, for the receiver we should never need to use SO_REUSEPORT
for these encapsulation sockets. If we're getting contention on a
single socket that should be resolved in the kernel (I believe Eric
was looking at eliminate lookup costs for these).

Tom

> IP 192.168.31.18.45367 > 192.168.31.17.4789: UDP, length 62
> IP 192.168.31.18.45515 > 192.168.31.17.4789: UDP, length 62
> IP 192.168.31.17.54206 > 192.168.31.18.4789: UDP, length 26814
> IP 192.168.31.18.45367 > 192.168.31.17.4789: UDP, length 62
> IP 192.168.31.18.45515 > 192.168.31.17.4789: UDP, length 62
> IP 192.168.31.17.50795 > 192.168.31.18.4789: UDP, length 25498
> IP 192.168.31.17.50795 > 192.168.31.18.4789: UDP, length 64922
> IP 192.168.31.17.50795 > 192.168.31.18.4789: UDP, length 64922
> IP 192.168.31.18.45367 > 192.168.31.17.4789: UDP, length 62
> IP 192.168.31.17.54206 > 192.168.31.18.4789: UDP, length 38170
>
>> This patch removes the argument and and adds a nocheck argument
>> for checksum setting. This eliminates the unnecessary dependency
>> on a UDP socket for UDP tunnel transmit.
>>
>> Signed-off-by: Tom Herbert <therbert@google.com>
>> ---
>>  drivers/net/vxlan.c       | 10 ++++++----
>>  include/net/udp_tunnel.h  | 16 ++++++++--------
>>  net/ipv4/udp_tunnel.c     | 12 ++++++------
>>  net/ipv6/ip6_udp_tunnel.c | 12 ++++++------
>>  4 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 6b6b456..4fb4205 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -1763,8 +1763,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
>>
>>         skb_set_inner_protocol(skb, htons(ETH_P_TEB));
>>
>> -       udp_tunnel6_xmit_skb(vs->sock, dst, skb, dev, saddr, daddr, prio,
>> -                            ttl, src_port, dst_port);
>> +       udp_tunnel6_xmit_skb(dst, skb, dev, saddr, daddr, prio,
>> +                            ttl, src_port, dst_port,
>> +                            udp_get_no_check6_tx(vs->sock->sk));
>>         return 0;
>>  err:
>>         dst_release(dst);
>> @@ -1842,8 +1843,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
>>
>>         skb_set_inner_protocol(skb, htons(ETH_P_TEB));
>>
>> -       return udp_tunnel_xmit_skb(vs->sock, rt, skb, src, dst, tos,
>> -                                  ttl, df, src_port, dst_port, xnet);
>> +       return udp_tunnel_xmit_skb(rt, skb, src, dst, tos,
>> +                                  ttl, df, src_port, dst_port, xnet,
>> +                                  vs->sock->sk->sk_no_check_tx);
>>  }
>>  EXPORT_SYMBOL_GPL(vxlan_xmit_skb);
>>
>> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
>> index 2a50a70..1a20d33 100644
>> --- a/include/net/udp_tunnel.h
>> +++ b/include/net/udp_tunnel.h
>> @@ -77,17 +77,17 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>>                            struct udp_tunnel_sock_cfg *sock_cfg);
>>
>>  /* Transmit the skb using UDP encapsulation. */
>> -int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
>> -                       struct sk_buff *skb, __be32 src, __be32 dst,
>> -                       __u8 tos, __u8 ttl, __be16 df, __be16 src_port,
>> -                       __be16 dst_port, bool xnet);
>> +int udp_tunnel_xmit_skb(struct rtable *rt, struct sk_buff *skb,
>> +                       __be32 src, __be32 dst, __u8 tos, __u8 ttl,
>> +                       __be16 df, __be16 src_port, __be16 dst_port,
>> +                       bool xnet, bool nocheck);
>>
>>  #if IS_ENABLED(CONFIG_IPV6)
>> -int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
>> -                        struct sk_buff *skb, struct net_device *dev,
>> -                        struct in6_addr *saddr, struct in6_addr *daddr,
>> +int udp_tunnel6_xmit_skb(struct dst_entry *dst, struct sk_buff *skb,
>> +                        struct net_device *dev, struct in6_addr *saddr,
>> +                        struct in6_addr *daddr,
>>                          __u8 prio, __u8 ttl, __be16 src_port,
>> -                        __be16 dst_port);
>> +                        __be16 dst_port, bool nocheck);
>>  #endif
>>
>>  void udp_tunnel_sock_release(struct socket *sock);
>> diff --git a/net/ipv4/udp_tunnel.c b/net/ipv4/udp_tunnel.c
>> index 9996e63..c83b354 100644
>> --- a/net/ipv4/udp_tunnel.c
>> +++ b/net/ipv4/udp_tunnel.c
>> @@ -75,10 +75,10 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>>  }
>>  EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
>>
>> -int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
>> -                       struct sk_buff *skb, __be32 src, __be32 dst,
>> -                       __u8 tos, __u8 ttl, __be16 df, __be16 src_port,
>> -                       __be16 dst_port, bool xnet)
>> +int udp_tunnel_xmit_skb(struct rtable *rt, struct sk_buff *skb,
>> +                       __be32 src, __be32 dst, __u8 tos, __u8 ttl,
>> +                       __be16 df, __be16 src_port, __be16 dst_port,
>> +                       bool xnet, bool nocheck)
>>  {
>>         struct udphdr *uh;
>>
>> @@ -90,9 +90,9 @@ int udp_tunnel_xmit_skb(struct socket *sock, struct rtable *rt,
>>         uh->source = src_port;
>>         uh->len = htons(skb->len);
>>
>> -       udp_set_csum(sock->sk->sk_no_check_tx, skb, src, dst, skb->len);
>> +       udp_set_csum(nocheck, skb, src, dst, skb->len);
>>
>> -       return iptunnel_xmit(sock->sk, rt, skb, src, dst, IPPROTO_UDP,
>> +       return iptunnel_xmit(skb->sk, rt, skb, src, dst, IPPROTO_UDP,
>>                              tos, ttl, df, xnet);
>>  }
>>  EXPORT_SYMBOL_GPL(udp_tunnel_xmit_skb);
>> diff --git a/net/ipv6/ip6_udp_tunnel.c b/net/ipv6/ip6_udp_tunnel.c
>> index 8db6c98..32d9b26 100644
>> --- a/net/ipv6/ip6_udp_tunnel.c
>> +++ b/net/ipv6/ip6_udp_tunnel.c
>> @@ -62,14 +62,14 @@ error:
>>  }
>>  EXPORT_SYMBOL_GPL(udp_sock_create6);
>>
>> -int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
>> -                        struct sk_buff *skb, struct net_device *dev,
>> -                        struct in6_addr *saddr, struct in6_addr *daddr,
>> -                        __u8 prio, __u8 ttl, __be16 src_port, __be16 dst_port)
>> +int udp_tunnel6_xmit_skb(struct dst_entry *dst, struct sk_buff *skb,
>> +                        struct net_device *dev, struct in6_addr *saddr,
>> +                        struct in6_addr *daddr,
>> +                        __u8 prio, __u8 ttl, __be16 src_port,
>> +                        __be16 dst_port, bool nocheck)
>>  {
>>         struct udphdr *uh;
>>         struct ipv6hdr *ip6h;
>> -       struct sock *sk = sock->sk;
>>
>>         __skb_push(skb, sizeof(*uh));
>>         skb_reset_transport_header(skb);
>> @@ -85,7 +85,7 @@ int udp_tunnel6_xmit_skb(struct socket *sock, struct dst_entry *dst,
>>                             | IPSKB_REROUTED);
>>         skb_dst_set(skb, dst);
>>
>> -       udp6_set_csum(udp_get_no_check6_tx(sk), skb, saddr, daddr, skb->len);
>> +       udp6_set_csum(nocheck, skb, saddr, daddr, skb->len);
>>
>>         __skb_push(skb, sizeof(*ip6h));
>>         skb_reset_network_header(skb);
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 2/2] vxlan: Eliminate dependency on UDP socket in transmit path

[Thomas Graf]

On 01/20/15 at 09:29am, Tom Herbert wrote:
> I didn't see any reason to preclude that, if it needs to be symmetric
> in that case it can be forced at the configuration. Being able to
> receive RCO but not have to send it to certain peers is important use
> case. You may want to consider this also for GBP if there are cases
> where we accept GBP from different peers, but only send it to certain
> ones.

I think asymmetric configurations are fine, in particular
receive-only. I was reluctant to the send-only scenario initially
as I would expect a VTEP sending RCO frames on UDP dport 8472 to
also always be able to accept RCO frames on that port. I can't
come up with any specific cases where this would lead to problems
though so I have no objections.

As for GBP, as processing of the policy group requires additional
iptables or OVS rules anyway, such behaviour would be implemented
in those rules by either ignoring the mark or dropping such frames.

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Or Gerlitz]

On Tue, Jan 20, 2015 at 7:36 PM, Tom Herbert <therbert@google.com> wrote:
> On Sun, Jan 18, 2015 at 2:43 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Sat, Jan 17, 2015 at 8:18 PM, Tom Herbert <therbert@google.com> wrote:
>>> The UDP tunnel transmit functions udp_tunnel_xmit_skb and
>>> udp_tunnel6_xmit_skb include a socket argument. The socket being
>>> passed to the functions (from VXLAN) is a UDP created for receive
>>> side. The only thing that the socket is used for in the transmit
>>> functions is to get the setting for checksum (enabled or zero).
>>
>> Tom, just to clarify - re the sockets usage in the transmit side,
>> somewhere bind or alike is done on them such that we have multiple
>> source UDP ports for given host VXLAN traffic. Here for example the
>> sender host is 192.168.31.17 and two ports are seen here 54206 and
>> 50795.
>>
>> Just wanted to make sure this series doesn't change that, since if
>> this is the case, we introduce here a regression w.r.t RSS hash
>> spreading from the outer UDP header data at the receiver side (which
>> is the right thing to do, per your LKS session...)
>>
> Hi Or,
>
> Using or not using a socket on transmit should have no bearing to the
> receive side. RSS works based on the hash of the UDP 5-tuple which
> should include a source port set to a value for the inner flow. Since
> the UDP socket is unconnected it should have no bearing on RFS or XPS
> either.

Hi Tom,

You say "which should include a source port set to a value for the
inner flow", well this series doesn't add such logic, nor I am fully
clear what piece exactly is responsible for the fact that I see
multiple source udp ports used from vxlan traffic flowing out of a
certain host. I just wanted to make sure that these patches don't
introduce a regression w.r.t to the **current** (not future) state of
things, can you confirm this?

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Tom Herbert]

On Tue, Jan 20, 2015 at 1:47 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Tue, Jan 20, 2015 at 7:36 PM, Tom Herbert <therbert@google.com> wrote:
>> On Sun, Jan 18, 2015 at 2:43 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Sat, Jan 17, 2015 at 8:18 PM, Tom Herbert <therbert@google.com> wrote:
>>>> The UDP tunnel transmit functions udp_tunnel_xmit_skb and
>>>> udp_tunnel6_xmit_skb include a socket argument. The socket being
>>>> passed to the functions (from VXLAN) is a UDP created for receive
>>>> side. The only thing that the socket is used for in the transmit
>>>> functions is to get the setting for checksum (enabled or zero).
>>>
>>> Tom, just to clarify - re the sockets usage in the transmit side,
>>> somewhere bind or alike is done on them such that we have multiple
>>> source UDP ports for given host VXLAN traffic. Here for example the
>>> sender host is 192.168.31.17 and two ports are seen here 54206 and
>>> 50795.
>>>
>>> Just wanted to make sure this series doesn't change that, since if
>>> this is the case, we introduce here a regression w.r.t RSS hash
>>> spreading from the outer UDP header data at the receiver side (which
>>> is the right thing to do, per your LKS session...)
>>>
>> Hi Or,
>>
>> Using or not using a socket on transmit should have no bearing to the
>> receive side. RSS works based on the hash of the UDP 5-tuple which
>> should include a source port set to a value for the inner flow. Since
>> the UDP socket is unconnected it should have no bearing on RFS or XPS
>> either.
>
> Hi Tom,
>
> You say "which should include a source port set to a value for the
> inner flow", well this series doesn't add such logic, nor I am fully
> clear what piece exactly is responsible for the fact that I see
> multiple source udp ports used from vxlan traffic flowing out of a
> certain host. I just wanted to make sure that these patches don't
> introduce a regression w.r.t to the **current** (not future) state of
> things, can you confirm this?

vxlan_xmit_one calls udp_flow_src_port to get a source port value
based on the encapsulated flow.

Re: [PATCH net-next 2/2] vxlan: Eliminate dependency on UDP socket in transmit path

[Tom Herbert]

On Tue, Jan 20, 2015 at 10:13 AM, Thomas Graf <tgraf@suug.ch> wrote:
> On 01/20/15 at 09:29am, Tom Herbert wrote:
>> I didn't see any reason to preclude that, if it needs to be symmetric
>> in that case it can be forced at the configuration. Being able to
>> receive RCO but not have to send it to certain peers is important use
>> case. You may want to consider this also for GBP if there are cases
>> where we accept GBP from different peers, but only send it to certain
>> ones.
>
> I think asymmetric configurations are fine, in particular
> receive-only. I was reluctant to the send-only scenario initially
> as I would expect a VTEP sending RCO frames on UDP dport 8472 to
> also always be able to accept RCO frames on that port. I can't
> come up with any specific cases where this would lead to problems
> though so I have no objections.
>
> As for GBP, as processing of the policy group requires additional
> iptables or OVS rules anyway, such behaviour would be implemented
> in those rules by either ignoring the mark or dropping such frames.

It's risky from a protocol perspective to assume that sending
something will be properly ignored. Just because we're willing to
receive something, doesn't mean we necessarily want to send it--
that's the robustness principle :-)

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Or Gerlitz]

On Wed, Jan 21, 2015 at 12:24 AM, Tom Herbert <therbert@google.com> wrote:
> vxlan_xmit_one calls udp_flow_src_port to get a source port value
> based on the encapsulated flow.

OK, got it -- does a by-product of invoking udp_flow_src_port on the
skb is having an hash mark on the skb which is based on the inner
packet and can (is?) used for TX queue selection over MQ devices?

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Tom Herbert]

On Wed, Jan 21, 2015 at 12:57 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Wed, Jan 21, 2015 at 12:24 AM, Tom Herbert <therbert@google.com> wrote:
>> vxlan_xmit_one calls udp_flow_src_port to get a source port value
>> based on the encapsulated flow.
>
> OK, got it -- does a by-product of invoking udp_flow_src_port on the
> skb is having an hash mark on the skb which is based on the inner
> packet and can (is?) used for TX queue selection over MQ devices?

Yes, that should be fine. The hash can be used all the way up to TCP
and save in the TCP socket as the rxhash for a connection. This was
RFS, XPS, etc. will work for TCP over an encapsulation.

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Or Gerlitz]

On Thu, Jan 22, 2015 at 12:24 AM, Tom Herbert <therbert@google.com> wrote:
> On Wed, Jan 21, 2015 at 12:57 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Wed, Jan 21, 2015 at 12:24 AM, Tom Herbert <therbert@google.com> wrote:
>>> vxlan_xmit_one calls udp_flow_src_port to get a source port value
>>> based on the encapsulated flow.
>>
>> OK, got it -- does a by-product of invoking udp_flow_src_port on the
>> skb is having an hash mark on the skb which is based on the inner
>> packet and can (is?) used for TX queue selection over MQ devices?
>
> Yes, that should be fine. The hash can be used all the way up to TCP
> and save in the TCP socket as the rxhash for a connection. This was
> RFS, XPS, etc. will work for TCP over an encapsulation.


so.. can be used or is already used today? Also we're talking on the
TX path, so I wasn't sure to follow on your mentioning of RFS...

Re: [PATCH net-next 1/2] udp: Do not require sock in udp_tunnel_xmit_skb

[Tom Herbert]

On Wed, Jan 21, 2015 at 2:37 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Thu, Jan 22, 2015 at 12:24 AM, Tom Herbert <therbert@google.com> wrote:
>> On Wed, Jan 21, 2015 at 12:57 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Wed, Jan 21, 2015 at 12:24 AM, Tom Herbert <therbert@google.com> wrote:
>>>> vxlan_xmit_one calls udp_flow_src_port to get a source port value
>>>> based on the encapsulated flow.
>>>
>>> OK, got it -- does a by-product of invoking udp_flow_src_port on the
>>> skb is having an hash mark on the skb which is based on the inner
>>> packet and can (is?) used for TX queue selection over MQ devices?
>>
>> Yes, that should be fine. The hash can be used all the way up to TCP
>> and save in the TCP socket as the rxhash for a connection. This was
>> RFS, XPS, etc. will work for TCP over an encapsulation.
>
>
> so.. can be used or is already used today? Also we're talking on the
> TX path, so I wasn't sure to follow on your mentioning of RFS...

Yes. AFAIK all encapsulation implementations are single queue and
otherwise transparent to the application layer for the hash and queue
selection.

Is there a particular feature you're having trouble with?

Thanks,
Tom