[ovs-dev] [mpls v2 1/2] Implement OpenFlow support for MPLS, for up to 3 labels.

[Jesse Gross]

On Thu, Jan 9, 2014 at 9:17 AM, Ben Pfaff <blp at 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 <blp at 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 <blp at nicira.com>
>> > Co-authored-by: Simon Horman <horms at verge.net.au>
>> > Signed-off-by: Simon Horman <horms at 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.