From patchwork Wed Jun 18 08:18:56 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Simon Horman X-Patchwork-Id: 361343 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 0BB5C140077 for ; Wed, 18 Jun 2014 18:19:10 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933686AbaFRITB (ORCPT ); Wed, 18 Jun 2014 04:19:01 -0400 Received: from kirsty.vergenet.net ([202.4.237.240]:56211 "EHLO kirsty.vergenet.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756377AbaFRIS7 (ORCPT ); Wed, 18 Jun 2014 04:18:59 -0400 Received: from ayumi.isobedori.kobe.vergenet.net (p4005-ipbfp1104kobeminato.hyogo.ocn.ne.jp [122.23.15.5]) by kirsty.vergenet.net (Postfix) with ESMTP id 9542B25BE87; Wed, 18 Jun 2014 18:18:57 +1000 (EST) Received: by ayumi.isobedori.kobe.vergenet.net (Postfix, from userid 7100) id 4542FEDEC4F; Wed, 18 Jun 2014 17:18:56 +0900 (JST) Date: Wed, 18 Jun 2014 17:18:56 +0900 From: Simon Horman To: Jesse Gross Cc: "dev@openvswitch.org" , netdev , Ravi K , Joe Stringer , Thomas Graf Subject: Re: [PATCH v2.60] datapath: Add basic MPLS support to kernel Message-ID: <20140618081855.GC14968@verge.net.au> References: <1402050531-12599-1-git-send-email-horms@verge.net.au> <20140617132045.GA11070@verge.net.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Organisation: Horms Solutions Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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. > 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? --- 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 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 */