On Fri, Jan 10, 2014 at 4:10 PM, Ben Pfaff <b...@nicira.com> wrote: > On Fri, Jan 10, 2014 at 03:50:09PM -0800, Jesse Gross wrote: >> On Thu, Jan 9, 2014 at 9:17 AM, Ben Pfaff <b...@nicira.com> wrote: >> > On Thu, Jan 02, 2014 at 11:51:15AM -0500, Jesse Gross wrote: >> >> On Sun, Dec 29, 2013 at 2:50 AM, Ben Pfaff <b...@nicira.com> wrote: >> >> * I'm not sure that flow_count_mpls_labels() is megaflow-safe since it >> >> reads fields without updating the wildcards. I think the only real way >> >> to handle this is with automatic tracking. >> > >> > By "automatic tracking" do you mean something other than just updating >> > the wildcard mask in flow_count_mpls_labels()? Presumably the core of >> > that is pretty easy. I haven't thought it through carefully, but I've >> > done a prototype and appended it to this message. (I'm also keeping >> > the branch at >> > https://github.com/blp/ovs-reviews.git mpls >> > up to date with my changes as I go. I'm happy to repost the overall >> > patch, by request.) >> >> I was thinking more of a set of macros that automatically unwildcards >> fields when they are read in a flow plus using something like sparse >> to warn if flows fields are accessed directly. It otherwise seems too >> easy to forget a field and have difficult to track down bugs. It might >> even result in better masks in some case by tracking exactly what we >> use since we always unwildcard some fields for simplicity. > > Something like that might be worthwhile, but I think that it is > orthogonal to MPLS, because we could implement it with or without > MPLS. (Unless you are suggesting that MPLS is somehow a "tipping > point" that makes manual wildcard management infeasible?)
I agree that it's orthogonal and there's nothing particularly MPLS specific about it but it does seem important in general. >> >> Other things, assuming that the goal is to eventually hook this up to >> >> the kernel patches: >> >> >> >> * How do we handle cases where userspace supports parsing more labels >> >> than the kernel (which is actually true at the moment)? I think that >> >> it would just fail at flow installation time right now. >> > >> > For matching (parsing) or for actions (pushing/popping)? For matching, >> > I guess that we could handle this using the existing "fitness" >> > mechanism. For actions, userspace could detect that the MPLS depth >> > exceeds what the kernel supports (I think we could probe for that pretty >> > easily) and fall back to the recently introduced "needs_help" mechanism >> > (see struct dpif_execute and dpif_execute_with_help()). >> >> I was thinking about matching here. If the kernel supplies more labels >> than userspace can handle then that is easy to detect. However, how to >> do we know if the kernel parsed fewer labels than userspace would have >> for a given packet? I guess we could check to see if the last label >> has the BoS bit set but then we'd have to make sure it still handles >> malformed or truncated packets as well. > > My plan was to check for the BoS bit. > > I suspect we have a lot of existing bugs in related areas. We need to > add some tests here. > > Do you have a specific scenario in mind, or is this just something to > think through in implementation? No, nothing really specific other than the goal of trying to be somewhat flexible and agnostic even in the case of malformed packets. >> >> * Somewhat related to the above, the kernel's action validator doesn't >> >> trust userspace to provide a valid EtherType for pop actions, so it >> >> doesn't allow multiple consecutive pops in cases where there are more >> >> tags than it knows about (which is 1). >> > >> > I guess this could be handled the same as the previous issue: detect and >> > avoid. >> >> I agree that actions are fairly easy to probe. We should probably >> support the same number of labels in the kernel as we do in userspace, >> at least at the beginning. That will avoid the problem completely in >> the vast majority of cases. > > Yes. > > I spoke to Bruce Davie about this a few days ago. He said that > hardware that he is familiar with supports up to 5 labels internally, > for reasons that were well thought out at the time, so I'm inclined to > have an initial userspace limit of 5 labels (instead of the 3 in my > initial patch). > > On the kernel side, it looks like we have 76 bytes in struct > sw_flow_key following 'eth', which is enough room for 19 labels. That > is not much of a limitation! Yeah, there's definitely plenty of space for MPLS labels so I'm not worried about it from the kernel perspective. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
