On Tue, Jun 7, 2016 at 11:09 AM, Johnson Li <johnson...@intel.com> wrote: > In this patch set, we implement the NSH metadata type 1 support for > the openvswtich. > Since the overlay could be VxLAN-GPE which is upstreamed by Jiri > at Linux kernel tree > commit <e1e5314de08ba6003b358125eafc9ad9e75a950c> > So we also add VxLAN-GPE support for openvswitch. > > Once add VxLAN-GPE support, the VxLAN port's ethernet type is set > to ARPHRD_NONE, so RAW protocol should be supported by the openvswitch. > This breaks the assumption that all packets should have a L2 header. > Simon upstreamed a patch set to add the raw protocol support at: > https://github.com/horms/openvswitch.git > This patch set depends on Simon's patch.
This series has too many dependencies on unmerged code to really make sense to review carefully at this point but I have some general comments: * Patches 1, 2, and 7 don't appear to have made it to the mailing list. * I really want to see support for MD type 2 metadata in the initial patch series. Not only do I think that it might affect how the overall code is structured but I've seen too many shortcut implementations where this gets left behind. * Related to that, it seems weird to me that md_type is exposed as an OpenFlow field. It seems like it should be implied by the fields that are actually used. (This is part of the reason why I'd like to see support for both types implemented initially, since this is exposing an interface that we won't be able to change in the future if we get it wrong now.) * Please structure kernel code as patches that are submitted upstream and then direct backports of those patches. * It's not clear to me exactly what scenarios this is targeting. My assumption is that the main one is nested inside of VXLAN GPE. It looks like it is trying to be decoupled enough to also support other use cases such as directly nesting in an Ethernet header. However, I'm not sure that this is fully implemented and might just result in malformed packets. * This adds a fair amount of additional flow state and the addition of MD type 2 will add quite a bit more. In most cases this is redundant with state already allocated for tunnel metadata. I'm aware that the design of NSH tries to operate at multiple layers, which makes it tricky to integrate it properly with tunneling. However, I don't think that doubling this amount of state is reasonable considering that it's pretty unlikely that both will be used at the same time, so I'd like for you to find a clear way to handle this. This is especially important in the kernel where increasing the size of the flow has a per-packet cost. * Related to the above, but for a different reason, please try to reuse as many existing constructs as possible, such as the OAM flag. This will enable us to have one set of code that operates on this rather than multiple given the concept is the same. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
