Skip to contentSkip to conversation

openvswitch: mpls fix and clean up

PATCH In review Jiri Benc 4 participants 10 messages Sep 30, 2016 – Oct 3, 2016

Subsystems (3)
Networking Network drivers Openvswitch
lore
Share & tools
lore

Jiri Benc

author
#0 lore raw

v2 cover openvswitch: mpls fix and clean up

Convert to the new mpls skb layout the last remaining place in openvswitch, forgotten on the mpls GSO rework. The GSO rework also allows for some cleanup in the third patch.

Jiri Benc (3):
openvswitch: mpls: set network header correctly on key extract
mpls: move mpls_hdr to a common location
openvswitch: use mpls_hdr

 include/net/mpls.h        | 15 ++++++---------
 net/mpls/internal.h       | 10 +---------
 net/openvswitch/actions.c | 24 ++++++++++++------------
 net/openvswitch/flow.c    | 11 +++--------
 4 files changed, 22 insertions(+), 38 deletions(-)
Signature · 1 line
1.8.3.1
author
#2 lore raw

PATCH v2 2/3 mpls: move mpls_hdr to a common location

This will be also used by openvswitch.

Signed-off-by Jiri Benc <[email protected]>

---
v2: new in v2

---
 include/net/mpls.h  |  9 +++++++++
 net/mpls/internal.h | 10 +---------
 2 files changed, 10 insertions(+), 9 deletions(-)
include/net/mpls.h +9 -0
index 5b3b5addfb08..3ebbc0bb57ff 100644
@@ -19,12 +19,21 @@
 
 #define MPLS_HLEN 4
 
+struct mpls_shim_hdr {
+	__be32 label_stack_entry;
+};
+
 static inline bool eth_p_mpls(__be16 eth_type)
 {
 	return eth_type == htons(ETH_P_MPLS_UC) ||
 		eth_type == htons(ETH_P_MPLS_MC);
 }
 
+static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
+{
+	return (struct mpls_shim_hdr *)skb_network_header(skb);
+}
+
 /*
  * For non-MPLS skbs this will correspond to the network header.
  * For MPLS skbs it will be before the network_header as the MPLS
net/mpls/internal.h +1 -9
index 732a5c17e986..bdfef6c3271a 100644
@@ -1,9 +1,6 @@
 #ifndef MPLS_INTERNAL_H
 #define MPLS_INTERNAL_H
-
-struct mpls_shim_hdr {
-	__be32 label_stack_entry;
-};
+#include <net/mpls.h>
 
 struct mpls_entry_decoded {
 	u32 label;

@@ -93,11 +90,6 @@ struct mpls_route { /* next hop label forwarding entry */
 
 #define endfor_nexthops(rt) }
 
-static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
-{
-	return (struct mpls_shim_hdr *)skb_network_header(skb);
-}
-
 static inline struct mpls_shim_hdr mpls_entry_encode(u32 label, unsigned ttl, unsigned tc, bool bos)
 {
 	struct mpls_shim_hdr result;
Signature · 1 line
1.8.3.1
author
#7 lore raw

PATCH v2 3/3 openvswitch: use mpls_hdr

In reply to Jiri Benc

skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper instead.

Signed-off-by Jiri Benc <[email protected]>

---
v2: using the existing mpls_hdr instead of skb_network_header

---
 include/net/mpls.h        | 12 ------------
 net/openvswitch/actions.c | 24 ++++++++++++------------
 2 files changed, 12 insertions(+), 24 deletions(-)
include/net/mpls.h +0 -12
index 3ebbc0bb57ff..1dbc669b770e 100644
@@ -33,16 +33,4 @@ static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
 {
 	return (struct mpls_shim_hdr *)skb_network_header(skb);
 }
-
-/*
- * For non-MPLS skbs this will correspond to the network header.
- * For MPLS skbs it will be before the network_header as the MPLS
- * label stack lies between the end of the mac header and the network
- * header. That is, for MPLS skbs the end of the mac header
- * is the top of the MPLS label stack.
- */
-static inline unsigned char *skb_mpls_header(struct sk_buff *skb)
-{
-	return skb_mac_header(skb) + skb->mac_len;
-}
 #endif
net/openvswitch/actions.c +12 -12
index 863e992dfbc0..4e03f64709bc 100644
@@ -160,7 +160,7 @@ static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
 static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 		     const struct ovs_action_push_mpls *mpls)
 {
-	__be32 *new_mpls_lse;
+	struct mpls_shim_hdr *new_mpls_lse;
 
 	/* Networking stack do not allow simultaneous Tunnel and MPLS GSO. */
 	if (skb->encapsulation)

@@ -180,8 +180,8 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	skb_reset_mac_header(skb);
 	skb_set_network_header(skb, skb->mac_len);
 
-	new_mpls_lse = (__be32 *)skb_mpls_header(skb);
-	*new_mpls_lse = mpls->mpls_lse;
+	new_mpls_lse = mpls_hdr(skb);
+	new_mpls_lse->label_stack_entry = mpls->mpls_lse;
 
 	skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
 

@@ -202,7 +202,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	if (unlikely(err))
 		return err;
 
-	skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN);
+	skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
 
 	memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
 		skb->mac_len);

@@ -211,10 +211,10 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	skb_reset_mac_header(skb);
 	skb_set_network_header(skb, skb->mac_len);
 
-	/* skb_mpls_header() is used to locate the ethertype
-	 * field correctly in the presence of VLAN tags.
+	/* mpls_hdr() is used to locate the ethertype field correctly in the
+	 * presence of VLAN tags.
 	 */
-	hdr = (struct ethhdr *)(skb_mpls_header(skb) - ETH_HLEN);
+	hdr = (struct ethhdr *)((void *)mpls_hdr(skb) - ETH_HLEN);
 	update_ethertype(skb, hdr, ethertype);
 	if (eth_p_mpls(skb->protocol))
 		skb->protocol = ethertype;

@@ -226,7 +226,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
 		    const __be32 *mpls_lse, const __be32 *mask)
 {
-	__be32 *stack;
+	struct mpls_shim_hdr *stack;
 	__be32 lse;
 	int err;
 

@@ -234,16 +234,16 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	if (unlikely(err))
 		return err;
 
-	stack = (__be32 *)skb_mpls_header(skb);
-	lse = OVS_MASKED(*stack, *mpls_lse, *mask);
+	stack = mpls_hdr(skb);
+	lse = OVS_MASKED(stack->label_stack_entry, *mpls_lse, *mask);
 	if (skb->ip_summed == CHECKSUM_COMPLETE) {
-		__be32 diff[] = { ~(*stack), lse };
+		__be32 diff[] = { ~(stack->label_stack_entry), lse };
 
 		skb->csum = ~csum_partial((char *)diff, sizeof(diff),
 					  ~skb->csum);
 	}
 
-	*stack = lse;
+	stack->label_stack_entry = lse;
 	flow_key->mpls.top_lse = lse;
 	return 0;
 }
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH v2 1/3 openvswitch: mpls: set network header correctly on key extract

In reply to Jiri Benc

After the 48d2ab609b6b ("net: mpls: Fixups for GSO"), MPLS handling in openvswitch was changed to have network header pointing to the start of the MPLS headers and inner_network_header pointing after the MPLS headers.

However, key_extract was missed by the mentioned commit, causing incorrect headers to be set when a MPLS packet just enters the bridge or after it is recirculated.

Fixes 48d2ab609b6b ("net: mpls: Fixups for GSO")
Signed-off-by Jiri Benc <[email protected]>
Acked-by Pravin B Shelar <[email protected]>

---
v2: no changes

---
 net/openvswitch/flow.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
net/openvswitch/flow.c +3 -8
index 634cc10d6dee..c8c82e109c68 100644
@@ -633,12 +633,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 	} else if (eth_p_mpls(key->eth.type)) {
 		size_t stack_len = MPLS_HLEN;
 
-		/* In the presence of an MPLS label stack the end of the L2
-		 * header and the beginning of the L3 header differ.
-		 *
-		 * Advance network_header to the beginning of the L3
-		 * header. mac_len corresponds to the end of the L2 header.
-		 */
+		skb_set_inner_network_header(skb, skb->mac_len);
 		while (1) {
 			__be32 lse;
 

@@ -646,12 +641,12 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			if (unlikely(error))
 				return 0;
 
-			memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
+			memcpy(&lse, skb_inner_network_header(skb), MPLS_HLEN);
 
 			if (stack_len == MPLS_HLEN)
 				memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
 
-			skb_set_network_header(skb, skb->mac_len + stack_len);
+			skb_set_inner_network_header(skb, skb->mac_len + stack_len);
 			if (lse & htonl(MPLS_LS_S_MASK))
 				break;
Signature · 1 line
1.8.3.1
#4 lore raw

Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

In reply to Jiri Benc
Quoted 15 lines · Jiri Benc

This will be also used by openvswitch.

Signed-off-by Jiri Benc <[email protected]>

---
v2: new in v2

---
 include/net/mpls.h  |  9 +++++++++
 net/mpls/internal.h | 10 +---------
 2 files changed, 10 insertions(+), 9 deletions(-)
include/net/mpls.h +0 -0
index 5b3b5addfb08..3ebbc0bb57ff 100644

include/net/mpls.h line 25

Quoted 7 lines
include/net/mpls.h +4 -0
@@ -19,12 +19,21 @@
 
 #define MPLS_HLEN 4
 
+struct mpls_shim_hdr {
+       __be32 label_stack_entry;
+};
+

There is already struct mpls_label defined in uapi mpls header file. Is there reason for not using it here?

author
#5 lore raw

Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

Quoted 2 lines · pravin shelar

There is already struct mpls_label defined in uapi mpls header file. Is there reason for not using it here?

I think this should be unified as well. But it's for a different patch not related to this patchset. This patch just moves the existing mpls_hdr and mpls_shim_hdr definition to a new place.

In general, I think the mpls code deserves a round of overall cleanups. It should not block this patchset, though. It can be considered as a step towards that goal.

Jiri

Acked-by
#6 lore raw

Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

Quoted 4 lines · Jiri Benc
Quoted 2 lines · pravin shelar

There is already struct mpls_label defined in uapi mpls header file. Is there reason for not using it here?

Quoted 8 lines

I think this should be unified as well. But it's for a different patch not related to this patchset. This patch just moves the existing mpls_hdr and mpls_shim_hdr definition to a new place.

In general, I think the mpls code deserves a round of overall cleanups. It should not block this patchset, though. It can be considered as a step towards that goal.

Alright, I do not oppose this patch going in as it is.

Acked-by Pravin B Shelar <[email protected]>
maintainer
#9 lore raw
In reply to Jiri Benc

From: Jiri Benc <[email protected]>
Date: Fri, 30 Sep 2016 19:08:04 +0200

Quoted 3 lines

Convert to the new mpls skb layout the last remaining place in openvswitch, forgotten on the mpls GSO rework. The GSO rework also allows for some cleanup in the third patch.

Series applied, thanks.