[ovs-dev] [CudaMailTagged] [RFC PATCH 00/14] Add Network Service Header Support

[Jesse Gross]

On Tue, Jun 7, 2016 at 11:09 AM, Johnson Li <johnson.li at 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.