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 <ho...@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 <rke...@gmail.com> wrote: > > >> > On Thu, Sep 27, 2012 at 11:16 AM, Jesse Gross <je...@nicira.com> wrote: > > >> >> On Thu, Sep 27, 2012 at 9:26 AM, Ben Pfaff <b...@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 <yamah...@valinux.co.jp> > > >> >> >> Cc: Ravi K <rke...@gmail.com> > > >> >> > > > >> >> > I think Ravi's full last name is "Kerur". > > >> >> > > > >> >> >> Signed-off-by: Simon Horman <ho...@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
