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

Jesse Gross jesse at nicira.com
Thu Jan 17 17:16:36 UTC 2013


On Wed, Jan 16, 2013 at 8:54 PM, Simon Horman <horms at verge.net.au> wrote:
> On Wed, Jan 16, 2013 at 07:04:06PM -0800, Jesse Gross wrote:
>> 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.
>
> My understanding is that fundamentally OVS_KEY_ATTR_MPLS is used for as
> part of the userspace/kernel interface and that thus code on both sides
> uses it.  Likewise for OVS_ACTION_ATTR_PUSH_MPLS and OVS_ACTION_ATTR_POP_MPLS.

It is.  However, my understanding is that you were planning on
following Ben's suggestion to try to get the userspace portions in
first.  In that case, there is no userspace/kernel communication to
speak of.  If there is no benefit to exposing this interface then I'd
like to avoid to risk of misinterpretations in the future.

> Perhaps a good approach is to use a values towards the end of the possible
> space for OVS_KEY_ATTR_MPLS, OVS_ACTION_ATTR_PUSH_MPLS and
> OVS_ACTION_ATTR_POP_MPLS, as has been done for OVS_KEY_ATTR_TUN_ID, to
> reduce the likely hood of a clash with another identifier in the short to
> medium term.

The difference with OVS_KEY_ATTR_TUN_ID is that it actually is being
used for communication with the kernel.



More information about the dev mailing list