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>
