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

Jesse Gross jesse at nicira.com
Thu Jan 17 03:04:06 UTC 2013


On Wed, Jan 16, 2013 at 4:27 PM, Simon Horman <horms at verge.net.au> wrote:
> On Wed, Jan 16, 2013 at 11:24:42AM -0800, Jesse Gross wrote:
>> 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.
>
> OVS_KEY_ATTR_MPLS is used in userspace:
>
> * lib/odp-util.c uses it in flow key handling routines.
>   In particular in:
>   - ovs_key_attr_to_string()
>   - odp_flow_key_attr_len()
>   - format_odp_key_attr()
>   - parse_odp_key_attr()
>   - odp_flow_key_from_flow()
>   - parse_l3_onward()
>   - commit_mpls_action() [new]
>
> * lib/dpif-netdev.c uses it to allow the user-space datapath to handle
>   MPLS. In particular in execute_set_action().

It's the usage in the userspace/kernel interfaces that I'm concerned
about.  If we're saying that this is essentially an internal
identifier to userspace then there's no benefit to having the code
that communicates that value to the kernel at this time.  If we have
it but don't use then it's possible that there will be a conflict in
the future if there is a different user of this temporary identifier.



More information about the dev mailing list