From patchwork Fri May 20 05:29:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Horman X-Patchwork-Id: 624308 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3r9xM30lHQz9t5n for ; Fri, 20 May 2016 15:29:17 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=netronome-com.20150623.gappssmtp.com header.i=@netronome-com.20150623.gappssmtp.com header.b=vayNYHRD; dkim-atps=neutral Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id C540710632; Thu, 19 May 2016 22:29:16 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 39AA1101DE for ; Thu, 19 May 2016 22:29:15 -0700 (PDT) Received: from bar5.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 9FAB3420126 for ; Thu, 19 May 2016 23:29:14 -0600 (MDT) X-ASG-Debug-ID: 1463722152-09eadd02f2fd090001-byXFYA Received: from mx3-pf1.cudamail.com ([192.168.14.2]) by bar5.cudamail.com with ESMTP id hCYaLFpnQaX0wzyd (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Thu, 19 May 2016 23:29:12 -0600 (MDT) X-Barracuda-Envelope-From: simon.horman@netronome.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.2 Received: from unknown (HELO mail-pa0-f43.google.com) (209.85.220.43) by mx3-pf1.cudamail.com with ESMTPS (RC4-SHA encrypted); 20 May 2016 05:29:11 -0000 Received-SPF: neutral (mx3-pf1.cudamail.com: 209.85.220.43 is neither permitted nor denied by SPF record at spf.mandrillapp.com) X-Barracuda-RBL-Trusted-Forwarder: 209.85.220.43 Received: by mail-pa0-f43.google.com with SMTP id xk12so36127724pac.0 for ; Thu, 19 May 2016 22:29:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=jK7yH0s/b3yJKaIQzMbjRJQGf7iol4rqq6SuwnitAWc=; b=vayNYHRDiHn9wNvfrM+JsIUPNIenb9dL4IPHBP2dvSEvweVsZ7ZmVhuGLTAM+6IwVR sCPI7nksl84O4HZX/AgABbT8GZReMRZTn+2E4xaNkRHX1LDPYSV8NvLglwrjy7SDN9Bm 0V6vAS2WH/PjanijcLttUSRskqSVi3GO/JcBOEPFvLH3CSVhtrHmPy2O5UJoUBMKHL0/ Car7hX22fmq9ETO+ZwHfIOXckE0gJMmAez9ltQBc0drFJOx7qC43l0HXBsa38kt70Qk0 k26kb4JG4dOv3b7fVTeX/YIl5xJgM9P3afaAVDAwkFRPbi+bggcPTPL98yKxCjEFxB8l c5Cw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=jK7yH0s/b3yJKaIQzMbjRJQGf7iol4rqq6SuwnitAWc=; b=mpYuNp73D44sRsFYT1SYaaeUd043JH6zoOnCUkjR5HxSLObOMRIzoek3GatYBWHWuE bsHW9TCuJVdQGZqjTot0JTDpNU6rFDXi2OPt/gLemQ2RKZAb6LsD6dZt/lHZOIOHydd4 MXkbyHfUtxaJTn4913gh8kS3fRTsqz7VLsRpVYeJj7Xq8CfT/2fag2RYsjuSaA7NwpeQ 5GQEQ+0lfKRGK2W8fn4cAKJKJDizeiTYMMA+4qnCVxmdkbVFukYmqhN8p5j9tgiB9GO/ 9nLnmzUOnxVNMdIt2ZCBjJrHzk/IOZpn71rmcftHTfchs3WaftjWS0QS9e/RldzsSXjB 8yhA== X-Gm-Message-State: AOPr4FV1AZBFNkHDvurLZcHXmmF0XB0NI3pbtLsfsepHHoX8ESa33zYH2wsARlRPI11IA0VT X-Received: by 10.66.153.174 with SMTP id vh14mr1782328pab.131.1463722151217; Thu, 19 May 2016 22:29:11 -0700 (PDT) Received: from vergenet.net (reginn.isobedori.kobe.vergenet.net. [2001:470:4832:303:d63d:7eff:fe99:ac9d]) by smtp.gmail.com with ESMTPSA id t65sm7315705pfb.37.2016.05.19.22.29.05 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 May 2016 22:29:09 -0700 (PDT) Date: Fri, 20 May 2016 14:29:01 +0900 X-Barracuda-Apparent-Source-IP: 2001:470:4832:303:d63d:7eff:fe99:ac9d X-CudaMail-Envelope-Sender: simon.horman@netronome.com From: Simon Horman To: Jiri Benc X-CudaMail-MID: CM-V1-518068200 X-CudaMail-DTE: 051916 X-CudaMail-Originating-IP: 209.85.220.43 Message-ID: <20160520052858.GA15505@vergenet.net> X-ASG-Orig-Subj: [##CM-V1-518068200##]Re: [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support References: <1462347393-22354-1-git-send-email-simon.horman@netronome.com> <1462347393-22354-5-git-send-email-simon.horman@netronome.com> <20160517163250.7ead555e@griffin> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160517163250.7ead555e@griffin> User-Agent: Mutt/1.5.23 (2014-03-12) X-GBUdb-Analysis: 0, 209.85.220.43, Ugly c=0.385168 p=-0.285714 Source Normal X-MessageSniffer-Rules: 0-0-0-16109-c X-Barracuda-Connect: UNKNOWN[192.168.14.2] X-Barracuda-Start-Time: 1463722152 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.10 X-Barracuda-Spam-Status: No, SCORE=0.10 using global scores of TAG_LEVEL=3.5 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=4.0 tests=BSF_SC0_MISMATCH_TO, DKIM_SIGNED, RDNS_NONE X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.29748 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.00 BSF_SC0_MISMATCH_TO Envelope rcpt doesn't match header 0.00 DKIM_SIGNED Domain Keys Identified Mail: message has a signature 0.10 RDNS_NONE Delivered to trusted network by a host with no rDNS Cc: dev@openvswitch.org, netdev@vger.kernel.org Subject: Re: [ovs-dev] [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" Hi Jiri, On Tue, May 17, 2016 at 04:32:50PM +0200, Jiri Benc wrote: > Looking through the patchset again, this time more deeply. Sorry for > the delay. No need to be sorry, good things take time. > On Wed, 4 May 2016 16:36:30 +0900, Simon Horman wrote: > > +struct ovs_action_push_eth { > > + struct ovs_key_ethernet addresses; > > + __be16 eth_type; > > Extra spaces. Sorry about that. As per some earlier discussion I plan to remove the eth_type field entirely. > > > +static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key) > > +{ > > + skb_pull_rcsum(skb, ETH_HLEN); > > + skb_reset_mac_header(skb); > > + skb->mac_len -= ETH_HLEN; > > + > > + invalidate_flow_key(key); > > + return 0; > > +} > > There's a fundamental question here: how should pop_eth behave when > vlan tag is present? > > There are two options: either vlan is considered part of the Ethernet > header and pop_eth means implicitly resetting vlan tag, or packet can > have vlan tag even if it's not Ethernet. > > This patch seems to implement the first option; however, skb->vlan_tci > should be reset and pop_eth should check whether the vlan tag is > present in the frame (deaccelerated) and remove it if it is. Otherwise, > the behavior of pop_eth would be inconsistent. > > However, I'm not sure whether the second option does not make more > sense. It may, in fact, be needed - ARPHRD_NONE tunnel port could not > be set as an access port otherwise (unless I'm missing something). > > In that case, pop_eth will need to put the vlan tag to skb->vlan_tci if > it's in the frame itself. Also, push_vlan and pop_vlan would need to be > modified to work with is_layer3 packets. Good point. The second option does seem rather tempting although I'm not sure that it actually plays out in the access-port scenario at this time. > > +static int push_eth(struct sk_buff *skb, struct sw_flow_key *key, > > + const struct ovs_action_push_eth *ethh) > > +{ > > + int err; > > + > > + /* De-accelerate any hardware accelerated VLAN tag added to a previous > > + * Ethernet header */ > > + err = skb_vlan_deaccel(skb); > > Why? Just keep it in skb->vlan_tci. Agreed, this seems unnecessary. > > --- a/net/openvswitch/flow.c > > +++ b/net/openvswitch/flow.c > > @@ -468,28 +468,31 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) > > > > skb_reset_mac_header(skb); > > > > - /* Link layer. We are guaranteed to have at least the 14 byte Ethernet > > - * header in the linear data area. > > - */ > > - eth = eth_hdr(skb); > > - ether_addr_copy(key->eth.src, eth->h_source); > > - ether_addr_copy(key->eth.dst, eth->h_dest); > > + /* Link layer. */ > > + if (key->phy.is_layer3) { > > + key->eth.tci = 0; > > Could make sense to use skb->vlan_tci, see above. The incremental patch below is what I have so far. The patch to add skb_vlan_deaccel() should also be dropped. diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index c413c588a24f..6853ab008861 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2994,6 +2994,7 @@ int skb_vlan_pop(struct sk_buff *skb); int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci); struct sk_buff *pskb_extract(struct sk_buff *skb, int off, int to_copy, gfp_t gfp); +int skb_vlan_accel(struct sk_buff *skb); static inline int memcpy_from_msg(void *data, struct msghdr *msg, int len) { diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 7a1d48983f81..a36c7491f714 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -4482,12 +4482,28 @@ pull: return err; } -int skb_vlan_pop(struct sk_buff *skb) +/* If a vlan tag is present move it to hw accel tag */ +int skb_vlan_accel(struct sk_buff *skb) { u16 vlan_tci; __be16 vlan_proto; int err; + vlan_proto = skb->protocol; + err = __skb_vlan_pop(skb, &vlan_tci); + if (unlikely(err)) + return err; + + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci); + return 0; +} +EXPORT_SYMBOL(skb_vlan_accel); + +int skb_vlan_pop(struct sk_buff *skb) +{ + u16 vlan_tci; + int err; + if (likely(skb_vlan_tag_present(skb))) { skb->vlan_tci = 0; } else { @@ -4500,19 +4516,13 @@ int skb_vlan_pop(struct sk_buff *skb) if (err) return err; } - /* move next vlan tag to hw accel tag */ + if (likely((skb->protocol != htons(ETH_P_8021Q) && skb->protocol != htons(ETH_P_8021AD)) || skb->len < VLAN_ETH_HLEN)) return 0; - vlan_proto = skb->protocol; - err = __skb_vlan_pop(skb, &vlan_tci); - if (unlikely(err)) - return err; - - __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci); - return 0; + return skb_vlan_accel(skb); } EXPORT_SYMBOL(skb_vlan_pop); diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 7d9b2307d6eb..ad2331cde732 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -302,6 +302,17 @@ static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *flow_key, static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key) { + /* Push outermost VLAN tag to skb metadata unless a VLAN tag + * is already present there. + */ + if ((skb->protocol == htons(ETH_P_8021Q) || + skb->protocol == htons(ETH_P_8021AD)) && + !skb_vlan_tag_present(skb)) { + int err = skb_vlan_accel(skb); + if (unlikely(err)) + return err; + } + skb_pull_rcsum(skb, ETH_HLEN); skb_reset_mac_header(skb); skb->mac_len -= ETH_HLEN; @@ -314,13 +325,6 @@ static int push_eth(struct sk_buff *skb, struct sw_flow_key *key, const struct ovs_action_push_eth *ethh) { struct ethhdr *hdr; - int err; - - /* De-accelerate any hardware accelerated VLAN tag added to a previous - * Ethernet header */ - err = skb_vlan_deaccel(skb); - if (unlikely(err)) - return err; /* Add the new Ethernet header */ if (skb_cow_head(skb, ETH_HLEN) < 0) diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index 4d2698596033..fdefee776d4f 100644 --- a/net/openvswitch/flow.c +++ b/net/openvswitch/flow.c @@ -469,8 +469,10 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) skb_reset_mac_header(skb); /* Link layer. */ + key->eth.tci = 0; if (key->phy.is_layer3) { - key->eth.tci = 0; + if (skb_vlan_tag_present(skb)) + key->eth.tci = htons(skb->vlan_tci); } else { eth = eth_hdr(skb); ether_addr_copy(key->eth.src, eth->h_source); @@ -481,7 +483,6 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) * update skb->csum here. */ - key->eth.tci = 0; if (skb_vlan_tag_present(skb)) key->eth.tci = htons(skb->vlan_tci); else if (eth->h_proto == htons(ETH_P_8021Q))