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?) > >> * 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. I agree that the situation differs. dec_ttl is the only exception we really handle, so it was the one that came to mind. > >> 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? > >> * 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! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
