From: Simon Horman <simon.horman@netronome•com>
To: Jiri Benc <jbenc@redhat•com>
Cc: netdev@vger•kernel.org, dev@openvswitch•org,
Lorand Jakab <lojakab@cisco•com>,
Thomas Morin <thomas.morin@orange•com>
Subject: Re: [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support
Date: Fri, 20 May 2016 14:29:01 +0900 [thread overview]
Message-ID: <20160520052858.GA15505@vergenet.net> (raw)
In-Reply-To: <20160517163250.7ead555e@griffin>
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))
next prev parent reply other threads:[~2016-05-20 5:29 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-04 7:36 [PATCH v9 net-next 0/7] openvswitch: support for layer 3 encapsulated packets Simon Horman
2016-05-04 7:36 ` [PATCH v9 net-next 1/7] net: add skb_vlan_deaccel helper Simon Horman
2016-05-04 7:36 ` [PATCH v9 net-next 2/7] openvswitch: set skb protocol when receiving on internal device Simon Horman
2016-05-04 7:36 ` [PATCH v9 net-next 3/7] openvswitch: add support to push and pop mpls for layer3 packets Simon Horman
[not found] ` <1462347393-22354-4-git-send-email-simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>
2016-05-05 17:35 ` pravin shelar
2016-05-06 4:33 ` Simon Horman
2016-05-04 7:36 ` [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support Simon Horman
[not found] ` <1462347393-22354-5-git-send-email-simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>
2016-05-05 17:37 ` pravin shelar
2016-05-06 5:57 ` Simon Horman
2016-05-06 9:25 ` Jiri Benc
2016-05-09 8:04 ` Simon Horman
[not found] ` <20160509080420.GA4470-IxS8c3vjKQDk1uMJSBkQmQ@public.gmane.org>
2016-05-10 12:01 ` Jiri Benc
2016-05-11 1:50 ` Simon Horman
[not found] ` <20160511015009.GB24436-IxS8c3vjKQDk1uMJSBkQmQ@public.gmane.org>
2016-05-11 3:06 ` Simon Horman
2016-05-11 14:09 ` Jiri Benc
2016-05-11 22:46 ` Simon Horman
2016-05-17 14:43 ` Jiri Benc
2016-05-18 2:18 ` Simon Horman
2016-05-11 13:57 ` Jiri Benc
2016-05-06 9:35 ` Jiri Benc
2016-05-09 8:18 ` Simon Horman
2016-05-10 0:16 ` [ovs-dev] " Yang, Yi Y
[not found] ` <79BBBFE6CB6C9B488C1A45ACD284F51913CB6446-0J0gbvR4kTggGBtAFL8yw7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
2016-05-10 12:07 ` Jiri Benc
2016-05-10 12:06 ` Jiri Benc
2016-05-11 3:28 ` Simon Horman
2016-05-11 14:10 ` Jiri Benc
2016-05-17 14:32 ` Jiri Benc
2016-05-20 5:29 ` Simon Horman [this message]
2016-05-20 8:00 ` Jiri Benc
2016-05-20 8:11 ` Simon Horman
2016-05-20 8:16 ` Simon Horman
[not found] ` <20160520081611.GB17561-IxS8c3vjKQDk1uMJSBkQmQ@public.gmane.org>
2016-05-20 8:39 ` Jiri Benc
2016-05-20 9:12 ` Simon Horman
2016-05-20 9:20 ` Jiri Benc
2016-05-20 10:14 ` Simon Horman
[not found] ` <1462347393-22354-1-git-send-email-simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>
2016-05-04 7:36 ` [PATCH v9 net-next 5/7] openvswitch: add layer 3 support to ovs_packet_cmd_execute() Simon Horman
[not found] ` <1462347393-22354-6-git-send-email-simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>
2016-05-17 14:51 ` Jiri Benc
2016-05-18 2:24 ` Simon Horman
2016-05-04 7:36 ` [PATCH v9 net-next 6/7] openvswitch: extend layer 3 support to cover non-IP packets Simon Horman
2016-05-04 7:36 ` [PATCH v9 net-next 7/7] openvswitch: use ipgre tunnel rather than gretap tunnel Simon Horman
[not found] ` <1462347393-22354-8-git-send-email-simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>
2016-05-05 21:45 ` pravin shelar
2016-05-06 6:54 ` Simon Horman
2016-05-06 9:15 ` Jiri Benc
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160520052858.GA15505@vergenet.net \
--to=simon.horman@netronome$(echo .)com \
--cc=dev@openvswitch$(echo .)org \
--cc=jbenc@redhat$(echo .)com \
--cc=lojakab@cisco$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=thomas.morin@orange$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox