Re: [PATCH net-next v12 2/9] openvswitch: add mac_proto field to the flow key

[Pravin Shelar]

On Mon, Oct 17, 2016 at 6:02 AM, Jiri Benc  wrote:
> Use a hole in the structure. We support only Ethernet so far and will add
> a support for L2-less packets shortly. We could use a bool to indicate
> whether the Ethernet header is present or not but the approach with the
> mac_proto field is more generic and occupies the same number of bytes in the
> struct, while allowing later extensibility. It also makes the code in the
> next patches more self explaining.
>
> It would be nice to use ARPHRD_ constants but those are u16 which would be
> waste. Thus define our own constants.
>
> Another upside of this is that we can overload this new field to also denote
> whether the flow key is valid. This has the advantage that on
> refragmentation, we don't have to reparse the packet but can rely on the
> stored eth.type. This is especially important for the next patches in this
> series - instead of adding another branch for L2-less packets before calling
> ovs_fragment, we can just remove all those branches completely.
>
> Signed-off-by: Jiri Benc 
> ---
> There are three possible approaches:
>
> (1) The one in this patch.
>
> (2) Use just a one bit flag indicating whether the packet is L3 or Ethernet
> (similar to the "is_layer3" bool in v11). The code would stay very
> similar to this patchset, the memory consumption would be the same.
>
> (3) Use value of 14 for MAC_PROTO_ETHERNET. It would simplify things nicely,
> as ovs_mac_header_len would be identical to ovs_key_mac_proto, saving
> one comparison. Of course, this would mean that if other L2 protocols
> are added in the future, they can only have L2 header length different
> than 14. Sounds hacky, although I kind of like this.
>
> After thinking about pros and cons, I implemented (1). Seems to be most
> clear of the three options. But I'm happy to implement (2) or (3) if it's
> deemed better.

I like approach taken by this patch.

Acked-by: Pravin B Shelar 

[PATCH net-next v12 2/9] openvswitch: add mac_proto field to the flow key

[Jiri Benc]

Use a hole in the structure. We support only Ethernet so far and will add
a support for L2-less packets shortly. We could use a bool to indicate
whether the Ethernet header is present or not but the approach with the
mac_proto field is more generic and occupies the same number of bytes in the
struct, while allowing later extensibility. It also makes the code in the
next patches more self explaining.

It would be nice to use ARPHRD_ constants but those are u16 which would be
waste. Thus define our own constants.

Another upside of this is that we can overload this new field to also denote
whether the flow key is valid. This has the advantage that on
refragmentation, we don't have to reparse the packet but can rely on the
stored eth.type. This is especially important for the next patches in this
series - instead of adding another branch for L2-less packets before calling
ovs_fragment, we can just remove all those branches completely.

Signed-off-by: Jiri Benc 
---
There are three possible approaches:

(1) The one in this patch.

(2) Use just a one bit flag indicating whether the packet is L3 or Ethernet
(similar to the "is_layer3" bool in v11). The code would stay very
similar to this patchset, the memory consumption would be the same.

(3) Use value of 14 for MAC_PROTO_ETHERNET. It would simplify things nicely,
as ovs_mac_header_len would be identical to ovs_key_mac_proto, saving
one comparison. Of course, this would mean that if other L2 protocols
are added in the future, they can only have L2 header length different
than 14. Sounds hacky, although I kind of like this.

After thinking about pros and cons, I implemented (1). Seems to be most
clear of the three options. But I'm happy to implement (2) or (3) if it's
deemed better.
---
 net/openvswitch/actions.c  | 14 +++---
 net/openvswitch/flow.c |  1 +
 net/openvswitch/flow.h | 22 ++
 net/openvswitch/flow_netlink.c |  5 +
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 49af167105d3..44144f914920 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -137,12 +137,12 @@ static struct deferred_action 
*add_deferred_actions(struct sk_buff *skb,
 
 static void invalidate_flow_key(struct sw_flow_key *key)
 {
-   key->eth.type = htons(0);
+   key->mac_proto |= SW_FLOW_KEY_INVALID;
 }
 
 static bool is_flow_key_valid(const struct sw_flow_key *key)
 {
-   return !!key->eth.type;
+   return !(key->mac_proto & SW_FLOW_KEY_INVALID);
 }
 
 static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
@@ -796,16 +796,8 @@ static void do_output(struct datapath *dp, struct sk_buff 
*skb, int out_port,
ovs_vport_send(vport, skb);
} else if (mru <= vport->dev->mtu) {
struct net *net = read_pnet(>net);
-   __be16 ethertype = key->eth.type;
 
-   if (!is_flow_key_valid(key)) {
-   if (eth_p_mpls(skb->protocol))
-   ethertype = skb->inner_protocol;
-   else
-   ethertype = vlan_get_protocol(skb);
-   }
-
-   ovs_fragment(net, vport, skb, mru, ethertype);
+   ovs_fragment(net, vport, skb, mru, key->eth.type);
} else {
kfree_skb(skb);
}
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 22087062bd10..96c8c4716603 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -751,6 +751,7 @@ int ovs_flow_key_extract(const struct ip_tunnel_info 
*tun_info,
key->phy.skb_mark = skb->mark;
ovs_ct_fill_key(skb, key);
key->ovs_flow_hash = 0;
+   key->mac_proto = MAC_PROTO_ETHERNET;
key->recirc_id = 0;
 
return key_extract(skb, key);
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index ae783f5c6695..f61cae7f9030 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -37,6 +37,12 @@
 
 struct sk_buff;
 
+enum sw_flow_mac_proto {
+   MAC_PROTO_NONE = 0,
+   MAC_PROTO_ETHERNET,
+};
+#define SW_FLOW_KEY_INVALID0x80
+
 /* Store options at the end of the array if they are less than the
  * maximum size. This allows us to get the benefits of variable length
  * matching for small options.
@@ -68,6 +74,7 @@ struct sw_flow_key {
u32 skb_mark;   /* SKB mark. */
u16 in_port;/* Input switch port (or DP_MAX_PORTS). 
*/
} __packed phy; /* Safe when right after 'tun_key'. */
+   u8 mac_proto;   /* MAC layer protocol (e.g. Ethernet). 
*/
u8 tun_proto;   /* Protocol of encapsulating tunnel. */
u32 ovs_flow_hash;  /* Datapath