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

Ben Pfaff blp at nicira.com
Thu Jan 9 17:17:34 UTC 2014


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.)

> * 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.

What do you think?

> 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()).

> * 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.

Your thoughts?

--8<--------------------------cut here-------------------------->8--

diff --git a/lib/flow.c b/lib/flow.c
index e51c83c..1f4af12 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1054,13 +1054,19 @@ flow_set_vlan_pcp(struct flow *flow, uint8_t pcp)
  * the maximum number of LSEs that can be stored in 'flow' is returned.
  */
 int
-flow_count_mpls_labels(const struct flow *flow)
+flow_count_mpls_labels(const struct flow *flow, struct flow_wildcards *wc)
 {
+    if (wc) {
+        wc->masks.dl_type = OVS_BE16_MAX;
+    }
     if (eth_type_mpls(flow->dl_type)) {
         int i;
         int len = ARRAY_SIZE(flow->mpls_lse);
 
         for (i = 0; i < len; i++) {
+            if (wc) {
+                wc->masks.mpls_lse[i] |= htonl(MPLS_BOS_MASK);
+            }
             if (flow->mpls_lse[i] & htonl(MPLS_BOS_MASK)) {
                 return i + 1;
             }
@@ -1077,10 +1083,11 @@ flow_count_mpls_labels(const struct flow *flow)
  */
 int
 flow_count_common_mpls_labels(const struct flow *flow_a,
-                              const struct flow *flow_b)
+                              const struct flow *flow_b,
+                              struct flow_wildcards *wc)
 {
-    int flow_a_n = flow_count_mpls_labels(flow_a);
-    int flow_b_n = flow_count_mpls_labels(flow_b);
+    int flow_a_n = flow_count_mpls_labels(flow_a, wc);
+    int flow_b_n = flow_count_mpls_labels(flow_b, wc);
     int min_n = MIN(flow_a_n, flow_b_n);
 
     if (min_n == 0) {
@@ -1092,8 +1099,11 @@ flow_count_common_mpls_labels(const struct flow *flow_a,
         int i;
 
         for (i = 0; i < min_n; i++) {
-            if (flow_a->mpls_lse[a_last - i] !=
-                flow_b->mpls_lse[b_last - i]) {
+            if (wc) {
+                wc->masks.mpls_lse[a_last - i] = OVS_BE32_MAX;
+                wc->masks.mpls_lse[b_last - i] = OVS_BE32_MAX;
+            }
+            if (flow_a->mpls_lse[a_last - i] != flow_b->mpls_lse[b_last - i]) {
                 break;
             } else {
                 common_n++;
@@ -1108,7 +1118,7 @@ void
 flow_push_mpls(struct flow *flow, ovs_be16 mpls_eth_type,
                struct flow_wildcards *wc)
 {
-    int n = flow_count_mpls_labels(flow);
+    int n = flow_count_mpls_labels(flow, wc);
 
     ovs_assert(n < ARRAY_SIZE(flow->mpls_lse));
 
@@ -1143,7 +1153,7 @@ flow_push_mpls(struct flow *flow, ovs_be16 mpls_eth_type,
 bool
 flow_pop_mpls(struct flow *flow, ovs_be16 eth_type, struct flow_wildcards *wc)
 {
-    int n = flow_count_mpls_labels(flow);
+    int n = flow_count_mpls_labels(flow, wc);
     int i;
 
     if (n == 0) {



More information about the dev mailing list