[ovs-dev] [PATCH 01/16] User-Space MPLS actions and matches

[Jesse Gross]

On Tue, Jan 15, 2013 at 11:05 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, Jan 08, 2013 at 02:46:02PM +0900, Simon Horman wrote:
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index 5e32965..b421753 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -282,6 +282,7 @@ enum ovs_key_attr {
>>       OVS_KEY_ATTR_ARP,       /* struct ovs_key_arp */
>>       OVS_KEY_ATTR_ND,        /* struct ovs_key_nd */
>>       OVS_KEY_ATTR_SKB_MARK,  /* u32 skb mark */
>> +     OVS_KEY_ATTR_MPLS,      /* struct ovs_key_mpls */
>>       OVS_KEY_ATTR_IPV4_TUNNEL,  /* struct ovs_key_ipv4_tunnel */
>>       OVS_KEY_ATTR_TUN_ID = 63,  /* be64 tunnel ID */
>>       __OVS_KEY_ATTR_MAX
>
> I'm pretty sure we shouldn't be inserting OVS_KEY_ATTR_MPLS in front of
> another value that we're already aiming to get upstream.  I'd suggest a
> value of 62.
>
> Jesse, does that sound reasonable to you?  Another alternative would be
> to avoid modifying <linux/openvswitch.h> entirely, one way or another,
> until we have real kernel support, which would be a little extra work.

It's not ideal but I think it's fine for the time being.
Incidentally, Pravin is working on something similar from the kernel
side to allow transformation of Netlink tunnel attributes into
something more efficient for packet processing.

To ensure that there is no leakage, I would do two things: wrap all
MPLS definitions in #ifndef __KERNEL__ blocks and check that we don't
have any handling code into odp-util.c.  That way if another new
attribute later starts using this temporary value then an old version
of OVS userspace will ignore it rather than trying to interprete it as
MPLS.

If we do that then we can just put the MPLS attribute directly after
IPV4_TUNNEL and new attributes would be inserted in between.  The MPLS
value could change but that shouldn't matter because there the value
would be entirely internal.  This seems easier to deal with than
allocating at both ends.

> I'm not sure why we have OVS_ACTION_ATTR_SET_MPLS instead of using
> OVS_ACTION_SET to set OVS_KEY_ATTR_MPLS.

For the PUSH/POP actions I think we also have the same issue with
inserting in the middle of the list and can use the same solution.  In
the comment about the EtherType above struct ovs_action_push_mpls I
would also say that non-MPLS EtherTypes will get rejected rather than
result in a corrupt packet since that seems like better behavior.