[ovs-dev] [PATCH v2.0] Non-Datapath MPLS actions and matches

[Simon Horman]

On Wed, Oct 03, 2012 at 12:19:09PM +0900, Simon Horman wrote:
> On Fri, Sep 28, 2012 at 04:20:51PM -0700, Jesse Gross wrote:
> > On Thu, Sep 27, 2012 at 5:44 PM, Simon Horman <horms at verge.net.au> wrote:
> > > On Thu, Sep 27, 2012 at 04:45:22PM -0700, Jesse Gross wrote:
> > >> On Thu, Sep 27, 2012 at 12:05 PM, ravi kerur <rkerur at gmail.com> wrote:
> > >> > On Thu, Sep 27, 2012 at 11:16 AM, Jesse Gross <jesse at nicira.com> wrote:
> > >> >> On Thu, Sep 27, 2012 at 9:26 AM, Ben Pfaff <blp at nicira.com> wrote:
> > >> >> > On Wed, Sep 26, 2012 at 04:13:41PM +0900, Simon Horman wrote:
> > >> >> >> This patch provides an implementation of the non-datapath portions
> > >> >> >> of MPLS matches and actions.
> > >> >> >>
> > >> >> >> This patch is based on top of Ben Pfaff's series,
> > >> >> >> "set-field action support"
> > >> >> >>
> > >> >> >> Cc: Isaku Yamahata <yamahata at valinux.co.jp>
> > >> >> >> Cc: Ravi K <rkerur at gmail.com>
> > >> >> >
> > >> >> > I think Ravi's full last name is "Kerur".
> > >> >> >
> > >> >> >> Signed-off-by: Simon Horman <horms at verge.net.au>
> > >> >> >
> > >> >> > Jesse, do you have any concerns about this?  I haven't read past the
> > >> >> > diffstat yet.  But if it seems like a reasonable intermediate approach
> > >> >> > then I'm happy to review it.
> > >> >>
> > >> >> I don't think there is inherently anything wrong in starting with a
> > >> >> userspace-only approach.  I have a couple of specific concerns based
> > >> >> on briefly skimming the patch:
> > >> >>  * It seems like this is really the userspace half of the code which
> > >> >> assumes that the kernel portions will still be doing the work on the
> > >> >> actual packet flows.  If that's the case then I don't think that
> > >> >> userspace support can go in independently.  Otherwise, userspace
> > >> >> should really be self-contained and setup slow-path flows to do the
> > >> >> work itself.
> > >> >
> > >> >
> > >> > mpls userspace is completely self-contained i.e. doesn't depend on OVS
> > >> > kernel code. I am saying this based on implementation and testing. During
> > >> > testing no OVS kernel module was loaded and testing such as ping, iperf,
> > >> > netperf and scp executed.
> > >>
> > >> At the very least, userspace-only code shouldn't add anything to
> > >> either the userspace/kernel interface or odp-util.c.  I also don't see
> > >> how any MPLS action will actually get processed.  I do see that MPLS
> > >> actions send packets to the local port and then install a flow but I
> > >> think there is a misunderstanding because that doesn't make a lot of
> > >> sense to me.
> > >
> > > That portion is probably my handiwork. Could you give some guidance on
> > > a method that would work? I'm more than happy to rework things.
> > 
> > Currently this is changing the userspace flow to reflect any
> > modifications that we want to make.  This is normally correct because
> > then later when we want to actually output a packet (where any
> > modifications would get noticed) we do a comparison from the last
> > output and the current flow and generate any kernel actions necessary.
> >  However, in this case we don't have any kernel actions to do MPLS
> > modifications so the flow changes just get ignored.
> > 
> > Instead, what we need to do is set up a slow path flow to explicitly
> > send all MPLS packets to userspace if there is any MPLS match or
> > action.  We don't currently have anything in this category but it
> > would use the same infrastructure as CFM and LACP which also need to
> > go to userspace since they are actually consumed there.  Once the MPLS
> > packets are in userspace, it should actually modify them directly
> > rather than just change the flow.
> 
> Is the implication that packet modifications for all actions for
> MPLS packets would need to occur in user-space? Or in other words,
> the implementation of all non-MPLS actions would need to
> be duplicated?

Hi Jesse,

I think you can ignore my question above as per my explanation
at the end of this email.

> > >> >>  * If it is truly userspace only, the handling of multiple levels of  tags
> > >> >> seems a little incomplete since we actually have the full packet.
> > >> >
> > >> >
> > >> > Can you elaborate  please?
> > >>
> > >> Multiple levels of tags aren't handled, for example, when you pop a tag.
> > >
> > > Do you think that adding support for multiple levels of tags to the kernel
> > > is appropriate. Or should such cases be bounced to user-space?
> > 
> > The kernel should support enough tags to handle the common cases.
> > Initially I would make this 1 but I could see it becoming 2 in the
> > future.  However, regardless of how many tags we support it's always
> > possible that a greater number of tags are used than we support.  I
> > can see three ways to handle this:
> > 
> > 1. Don't support it.  You can run into a similar problem with vlan
> > tags because it's possible to pop tags off and then do further
> > operations but the kernel doesn't look deep enough into the packet.
> > There isn't really a mechanism to deal with this today and to my
> > knowledge nobody has actually complained.
> > 
> > 2. Handle in userspace.  It's certainly a reasonable approach for corner cases.
> > 
> > 3. Some form of recirculation.  Allowing the kernel to look at a
> > packet, pop some tags off, and then look at it again allows processing
> > an infinite number of tags in a manner that won't significantly affect
> > throughput (although each round will result in more flow setups).  It
> > also allows use cases like pop MPLS tag and do IP routing, which is
> > both fairly common and can't be realistically achieved any other way
> > in kernel.
> > 
> > If the short term goal is a kernel implementation, I would do #1 since
> > it's much simpler, is a strict increase in functionality from what we
> > have today and doesn't preclude doing #3 in the future.  If you want
> > something more complete then you can handle the extra cases using #2.
> > 
> > If it's likely that we'll be using the userspace version for a while
> > then it makes sense to handle all of the various cases upfront because
> > we'll always have all of the packets and there's really no reason not
> > to.
> > 
> > So it depends what your goal is.
> 
> My thinking is in terms of doing #1 then #2.
> 
> #3 seems to add significant complexity to the kernel for corner cases.
> If it is useful for a variety of cases then it may be worth-while,
> but it seems to me to be overkill for MPLS alone.

I have reconsidered and my current thinking is now in terms of doing #1
and leaving the door open for #3.

With that in mind I plan to post a revised patch which implements
the non-datapath portions for MPLS actions and matches without attempting
to provide non-datapath implementations of those actions or matches.
That is, a user-space portion awaiting a data-path portion.

I believe this is consistent with my new thinking not to pursue #2