Re: [PATCH net-next v2] openvswitch: enable NSH support

[Jiri Pirko <jiri@resnulli.us>]

Fri, Aug 11, 2017 at 12:09:36PM CEST, jan.scheurich@ericsson.com wrote:
>> -----Original Message-----
>> From: Jiri Benc [mailto:jbenc@redhat.com]
>> Sent: Friday, 11 August, 2017 11:45
>> 
>> The context field does not apply to MD type 2. It looks wrong for the
>> context field to be included in netlink attribute for anything other
>> than MD type 1. Perhaps it needs to be put into a separate attribute,
>> too?
>> 
>> Note that I'm talking only about the uAPI. Internally, ovs can use
>> struct ovs_key_nsh that is MD type 1 only, there's no problem changing
>> that later. But for the user space interface, this needs to be clean.
>> This can be solved for example this way:
>> 
>> In include/uapi/linux/openvswitch.h:
>> 
>> struct ovs_key_nsh_base {
>> 	__u8 flags;
>> 	__u8 mdtype;
>> 	__u8 np;
>> 	__u8 pad;
>> 	__be32 path_hdr;
>> };
>> 
>> + one more netlink attribute carrying MD type 1 info. Will probably
>> require to change OVS_KEY_ATTR_NSH to a nested attribute etc.
>> 
>> In net/openvswitch/flow.h (or perhaps a different header would be more
>> appropriate?):
>> 
>> struct ovs_key_nsh {
>> 	struct ovs_key_nsh_base base;
>> 	__be32 context[4];
>> };
>> 
>> Plus needed conversions between OVS_KEY_ATTR_NSH and struct ovs_key_nsh
>> when interfacing between the kernel and user space.
>> 
>> That way, we can have MD type 1 support only for now while still being
>> allowed to redesign things in whatever way later.
>> 
>>  Jiri
>
>For the OVS_KEY_ATTR_NSH I agree to move the fixed size MD1 context headers from nsh_base to a separate struct and use nested netlink attributes.
>
>For OVS_ACTION_ATTR_PUSH_NSH attribute any metadata included is opaque to the datapath and should be copied as is into the packet header. I doubt that there is any benefit to model this with nested attributes for MD1 or MD2. This just adds complexity on sender and receiver side and requires updates in case there should be other MD types added later on.
>
>Unless someone can explain to me why the datapath should understand the internal structure/format of metadata in push_nsh, I would strongly vote to keep the metadata as variable length octet sequence in the non-structured OVS_ACTION_ATTR_PUSH_NSH

Could you please wrap lines at 72 chars? This is unreadable. Thanks!