[ovs-dev] [PATCH v2.60] datapath: Add basic MPLS support to kernel

[Simon Horman]

On Tue, Jun 17, 2014 at 11:02:21PM -0700, Jesse Gross wrote:
> I'm currently seeing this compiler error:
> 
>   CC [M]  /home/jesse/openvswitch/datapath/linux/gso.o
> /home/jesse/openvswitch/datapath/linux/gso.c: In function ‘tnl_skb_gso_segment’:
> /home/jesse/openvswitch/datapath/linux/gso.c:199:2: error: implicit
> declaration of function ‘skb_inner_mac_offset’
> [-Werror=implicit-function-declaration]
>   int mac_offset = skb_inner_mac_offset(skb);
>   ^
> /home/jesse/openvswitch/datapath/linux/gso.c:233:3: error: implicit
> declaration of function ‘OVS_GSO_CB’
> [-Werror=implicit-function-declaration]
>    if (OVS_GSO_CB(skb)->fix_segment)
>    ^
> /home/jesse/openvswitch/datapath/linux/gso.c:233:22: error: invalid
> type argument of ‘->’ (have ‘int’)
>    if (OVS_GSO_CB(skb)->fix_segment)
>                       ^
> /home/jesse/openvswitch/datapath/linux/gso.c:234:19: error: invalid
> type argument of ‘->’ (have ‘int’)
>     OVS_GSO_CB(skb)->fix_segment(skb);
> 
> This is on 3.13. I was originally planning on trying to fix this
> myself but I'm obviously just slowing things down at this point :)
> 
> This is a consequence of the recent extension of the versions that the
> compat code is covering.

I came up with two ways to resolve this problem.

1. Tweak the KERNEL_VERSION(3,12,0) line to KERNEL_VERSION(3,16,0)
   near the top of datapath/linux/compat/gso.h and update the comment
   a bit further down accordingly.

   I think this should be correct but it seems a but dirty
   to have struct ovs_gso_cb when it isn't really needed any more.

2. The following change which avoids using struct ovs_gso_cb after
   v3.12 and allows using skb_inner_mac_offset up to v3.16.

   This is my preferred approach.

diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
index dc1e537..8344293 100644
--- a/datapath/linux/compat/gso.c
+++ b/datapath/linux/compat/gso.c
@@ -190,6 +190,16 @@ static __be16 __skb_network_protocol(struct sk_buff *skb)
 	return type;
 }
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(3,12,0)
+static void tnl_fix_segment(struct sk_buff *skb)
+{
+	if (OVS_GSO_CB(skb)->fix_segment)
+		OVS_GSO_CB(skb)->fix_segment(skb);
+}
+#else
+static void tnl_fix_segment(struct sk_buff *skb) { }
+#endif
+
 static struct sk_buff *tnl_skb_gso_segment(struct sk_buff *skb,
 					   netdev_features_t features,
 					   bool tx_path)
@@ -230,8 +240,7 @@ static struct sk_buff *tnl_skb_gso_segment(struct sk_buff *skb,
 
 		memcpy(ip_hdr(skb), iph, pkt_hlen);
 		memcpy(skb->cb, cb, sizeof(cb));
-		if (OVS_GSO_CB(skb)->fix_segment)
-			OVS_GSO_CB(skb)->fix_segment(skb);
+		tnl_fix_segment(skb);
 
 		skb->protocol = proto;
 		skb = skb->next;
diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
index 1393173..b08ad41 100644
--- a/datapath/linux/compat/gso.h
+++ b/datapath/linux/compat/gso.h
@@ -54,12 +54,6 @@ static inline int skb_inner_network_offset(const struct sk_buff *skb)
 	return skb_inner_network_header(skb) - skb->data;
 }
 
-#define skb_inner_mac_offset rpl_skb_inner_mac_offset
-static inline int skb_inner_mac_offset(const struct sk_buff *skb)
-{
-	return skb_inner_mac_header(skb) - skb->data;
-}
-
 #define skb_reset_inner_headers rpl_skb_reset_inner_headers
 static inline void skb_reset_inner_headers(struct sk_buff *skb)
 {
@@ -76,6 +70,14 @@ int ip_local_out(struct sk_buff *skb);
 
 #endif /* 3.12 */
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(3,16,0)
+#define skb_inner_mac_offset rpl_skb_inner_mac_offset
+static inline int skb_inner_mac_offset(const struct sk_buff *skb)
+{
+	return skb_inner_mac_header(skb) - skb->data;
+}
+#endif
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(3,11,0)
 static inline void ovs_skb_init_inner_protocol(struct sk_buff *skb) {
 	OVS_GSO_CB(skb)->inner_protocol = htons(0);



I also ran into a new compile problem when rebasing. My proposed solution
is as follows:

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 64041ff..73c215b 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -382,7 +382,7 @@ static size_t key_attr_size(void)
 {
 	/* Whenever adding new OVS_KEY_ FIELDS, we should consider
 	 * updating this function.  */
-	BUILD_BUG_ON(OVS_KEY_ATTR_IPV4_TUNNEL != 21);
+	BUILD_BUG_ON(OVS_KEY_ATTR_IPV4_TUNNEL != 22);
 
 	return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
 		+ nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
@@ -397,6 +397,7 @@ static size_t key_attr_size(void)
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_SKB_MARK */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_DP_HASH */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_RECIRC_ID */
+		+ nla_total_size(4)   /* OVS_KEY_ATTR_MPLS */
 		+ nla_total_size(12)  /* OVS_KEY_ATTR_ETHERNET */
 		+ nla_total_size(2)   /* OVS_KEY_ATTR_ETHERTYPE */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_8021Q */

> One thing that comes to mind is how do have
> correct behavior but avoid forcing unnecessary software segmentation
> for tunnels on later kernels.

An interesting problem that I had not considered.

Is the problem related to the presence of MPLS or the fact
that the compatibility code is now used in newer kernels?