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

Simon Horman horms at verge.net.au
Thu Jan 17 00:27:19 UTC 2013


On Wed, Jan 16, 2013 at 03:49:00PM -0800, Ben Pfaff 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.
> > 
> > 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.
> 
> All this sounds reasonable to me.  Simon, will you take care of it?

Yes, of course.



More information about the dev mailing list