On Mon, Oct 17, 2016 at 6:02 AM, Jiri Benc <jb...@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 <jb...@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 <pshe...@ovn.org>