Re: [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support

[Simon Horman <simon.horman@netronome.com>]

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))