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

From: Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org>
To: Jiri Benc <jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: ovs dev <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
	Linux Kernel Network Developers
	<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Simon Horman
	<simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>
Subject: Re: [PATCH net-next v12 2/9] openvswitch: add mac_proto field to the flow key
Date: Tue, 18 Oct 2016 22:11:32 -0700	[thread overview]
Message-ID: <CAOrHB_BNMo5eoHPaSLrq8XViRDk7fj3NYywMQMGOhb8R2RcQSQ@mail.gmail.com> (raw)
In-Reply-To: <098a6ab19e26cf7b1cc16f57f5fc0cc019ca44d6.1476708212.git.jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

On Mon, Oct 17, 2016 at 6:02 AM, Jiri Benc <jbenc@redhat.com> 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 <jbenc@redhat.com>
> ---
> 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 <pshelar@ovn.org>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev