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've been a little frustrated with the current approach to MPLS, because it >> > seems quite difficult to understand. One particularly difficult bit for >> > me is the variables used during translation, e.g. mpls_depth_delta and >> > pre_push_mpls_lse. And what we end up with is support for a single MPLS >> > label, which I don't think is going to make any real-world users happy. >> > >> > This commit attempts to implement something easier to understand and more >> > powerful by just keeping track of all the labels in struct flow. >> > >> > Signed-off-by: Ben Pfaff <b...@nicira.com> >> > Co-authored-by: Simon Horman <ho...@verge.net.au> >> > Signed-off-by: Simon Horman <ho...@verge.net.au> >> >> I have some general comments: >> >> * 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. >> * It seems like we might want a harder failure mechanism for cases >> where we exceed the number of labels that we are able to handle. I >> don't think that this is the same as trying to change L4 ports when >> you don't have an L4 header because an MPLS packet could still be >> interpreted, just differently. > > I think you're right. It should be possible to find a small class of > errors statically so that we can reject them at flow table insertion > time, but I guess the more common errors cannot be found that way and so > maybe that check isn't worth it. > > I guess we could start by just dropping packets. We could add a more > sophisticated exception mechanism in the future (like the one for > dec_ttl) if it proved useful, but it's probably easier for flow tables > to just avoid this exception by checking for BOS bits. I agree that the controller should be able to avoid this error in the first place and that would be better. I'm not sure that the exception is really the same as in dec_ttl - that one is related to the network traffic but this one is related to the capabilities of the switch itself. >> 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. >> * 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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
