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

[Li, Johnson]

> >     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:
> 
Yes,  the patch set depends on Simon's patch set for both ovs kernel module and
The user space control plane. If Simon's patch set is not merged into the mainstream,
This patch would not work as expected.
> * Patches 1, 2, and 7 don't appear to have made it to the mailing list.
Ok, I will remove the patches for the kernel module. Yi Yang < yi.y.yang at intel.com> and
I have sent the patch for kernel net-next tree before for review. 
> * 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.
Some codes are compatible with MD type 2 and some are not. I will try to rework the codes
To support both MD type 1 and MD type 2 soon. 
> * 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.
Already done,  Yi has sent to kernel and also copied to OVS dev maillist. We can find the
Commit at http://openvswitch.org/pipermail/dev/2016-June/072209.html. 
>  * 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.
We are trying to use these code to work with most cases: Ether + NSH, 
VxLAN-GPE + NSH, VxLAN + Ether + NSH and GRE + NSH. But these depend on
Simon's patch to enable raw protocol support and implicit pop/push ether
header data path action.   
>  * 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.
Yes, I will try to do that. 
>  * 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.