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. >> >> * 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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
