[ovs-dev] [PATCH 4/4] User-Space MPLS actions and matches

Simon Horman horms at verge.net.au
Mon Oct 15 05:42:27 UTC 2012


On Fri, Oct 12, 2012 at 01:05:05PM +0900, Simon Horman wrote:
> On Fri, Oct 12, 2012 at 10:24:24AM +0900, Isaku Yamahata wrote:
> > On Fri, Oct 12, 2012 at 09:04:09AM +0900, Simon Horman wrote:
> > > On Thu, Oct 11, 2012 at 10:10:39PM +0900, Isaku Yamahata wrote:
> > > > On Thu, Oct 11, 2012 at 11:14:40AM +0900, Simon Horman wrote:
> > > > > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > > > > index 5a1d31c..5ffbaef 100644
> > > > > --- a/lib/odp-util.c
> > > > > +++ b/lib/odp-util.c
> > > > ...
> > > > > @@ -1911,6 +2045,33 @@ commit_vlan_action(const struct flow *flow, struct flow *base,
> > > > >  }
> > > > >  
> > > > >  static void
> > > > > +commit_mpls_action(const struct flow *flow, struct flow *base,
> > > > > +                   struct ofpbuf *odp_actions)
> > > > > +{
> > > > > +    if (flow->mpls_lse == base->mpls_lse) {
> > > > > +        return;
> > > > > +    }
> > > > 
> > > > Not only label value, but also The stack depth needs to be compared. 
> > > 
> > > I'm a little confused.
> > > 
> > > This series doesn't track the stack depth as it has an implicit
> > > limit of one label.
> > 
> > The following cases for example. 
> > (I might be wrong because it depends on how flow->mpls_lse is managed.)
> > 
> > outer A, B inner --push_mpls(A)--> outer A, A, B inner
> > outer A, B, C inner --pop_mpls(), set_mpls(B)--> outer A, C inner
> > 
> > I'm not sure if the latter case should be allowed with the scope of this
> > patch series, though.
> 
> I think it should, though I haven't thought about it in depth.
> 
> > mpls_lse = 0 value is internally used for special meaning. I suppose 0 is
> > also valid label value. But I'm not a MPLS expert.
> 
> I also think it is also a valid label.
> It would be nice to avoid using it as a special value.
> 
> > Need to track mpls stack depth or depth change?
> 
> Thanks, I see the problem now. I'll see about modifying the
> patch to track depth changes.

Hi Yamahata-san,

as the MPLS patch is rather large I am posting an incremental
patch below that I believe implements the stack tracking that you suggest.
It seems to be a good idea as illustrated by the corrections to the
test-suite.

My intention is to roll this change into the "User-Space MPLS actions and
matches" patch if it receives favourable review.

----------------------------------------------------------------
Simon Horman <horms at verge.net.au>

MPLS stack depth

* Track MPLS stack depth and use it to determine if the MPLS label should
  be pushed, popped or modified
* No longer use 0 as a special MPLS LSE value
* Correct BoS check logic in parse_remaining_mpls() as
  that function is short and otherwise modified
  by this change.
---
 lib/flow.c             |   20 ++++++++------------
 lib/flow.h             |    3 ++-
 lib/odp-util.c         |   19 ++++++++-----------
 lib/packets.c          |   25 ++++++++++++++++++++++++-
 lib/packets.h          |    2 ++
 ofproto/ofproto-dpif.c |   39 +++++++++++++++++++++++++++++----------
 tests/ofproto-dpif.at  |    6 +++---
 tests/test-bundle.c    |    1 +
 tests/test-multipath.c |    1 +
 utilities/ovs-dpctl.c  |    2 +-
 10 files changed, 79 insertions(+), 39 deletions(-)

diff --git a/lib/flow.c b/lib/flow.c
index 5291e19..d91201e 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -93,13 +93,13 @@ pull_icmpv6(struct ofpbuf *packet)
 }
 
 static void
-parse_remaining_mpls(struct ofpbuf *b)
+parse_remaining_mpls(struct ofpbuf *b, struct flow *flow)
 {
     /* Proceed with parsing remaining MPLS headers. */
     struct mpls_hdr *mh;
     while ((mh = ofpbuf_try_pull(b, sizeof *mh)) &&
-           (mh->mpls_lse & htonl(MPLS_BOS_MASK))) {
-        ;
+           !(mh->mpls_lse & htonl(MPLS_BOS_MASK))) {
+        flow->mpls_depth++;
     }
 }
 
@@ -111,6 +111,7 @@ parse_mpls(struct ofpbuf *b, struct flow *flow)
     if (b->size >= sizeof(struct mpls_hdr) + sizeof(ovs_be16)) {
         struct mpls_hdr *mh = ofpbuf_pull(b, sizeof *mh);
         flow->mpls_lse = mh->mpls_lse;
+        flow->mpls_depth++;
     }
 }
 
@@ -406,7 +407,7 @@ flow_extract(struct ofpbuf *packet, uint32_t skb_priority,
         packet->l2_5 = b.data;
         parse_mpls(&b, flow);
         if (!(flow->mpls_lse & htonl(MPLS_BOS_MASK))) {
-            parse_remaining_mpls(&b);
+            parse_remaining_mpls(&b, flow);
         }
         if (packet->size >= sizeof *ih &&
             IP_VER(ih->ip_ihl_ver) == IP_VERSION) {
@@ -580,7 +581,7 @@ flow_format(struct ds *ds, const struct flow *flow)
                   ETH_ADDR_ARGS(flow->dl_dst),
                   ntohs(flow->dl_type));
 
-    if (flow->mpls_lse) {
+    if (flow->mpls_depth) {
         ds_put_format(ds, ",mpls(");
         ds_put_format(ds, "label:%"PRIu32",tc:%d,ttl:%d,bos:%d",
                       mpls_lse_to_label(flow->mpls_lse),
@@ -883,13 +884,8 @@ flow_set_vlan_pcp(struct flow *flow, uint8_t pcp)
 void
 flow_set_mpls_label(struct flow *flow, ovs_be32 label)
 {
-    if (label == htonl(0)) {
-        flow->mpls_lse = htonl(0);
-    } else {
-        flow->mpls_lse &= ~htonl(MPLS_LABEL_MASK);
-        flow->mpls_lse |=
-            htonl((ntohl(label) << MPLS_LABEL_SHIFT) & MPLS_LABEL_MASK);
-    }
+    flow->mpls_lse &= ~htonl(MPLS_LABEL_MASK);
+    flow->mpls_lse |= htonl((ntohl(label) << MPLS_LABEL_SHIFT) & MPLS_LABEL_MASK);
 }
 
 /* Sets the MPLS TC that 'flow' matches to 'tc', which should be in the
diff --git a/lib/flow.h b/lib/flow.h
index dc02a89..f6d0ca2 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -91,7 +91,8 @@ struct flow {
     uint8_t arp_tha[6];         /* ARP/ND target hardware address. */
     uint8_t nw_ttl;             /* IP TTL/Hop Limit. */
     uint8_t nw_frag;            /* FLOW_FRAG_* flags. */
-    uint8_t zeros[4];           /* Must be zero. */
+    uint16_t mpls_depth;        /* Depth of MPLS stack. */
+    uint8_t zeros[2];           /* Must be zero. */
 };
 BUILD_ASSERT_DECL(sizeof(struct flow) % 4 == 0);
 
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 5ffbaef..cf8a531 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1395,7 +1395,7 @@ odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow)
         memcpy(arp_key->arp_tha, flow->arp_tha, ETH_ADDR_LEN);
     }
 
-    if (flow->mpls_lse != htonl(0)) {
+    if (flow->mpls_depth) {
         struct ovs_key_mpls *mpls_key;
 
         mpls_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_MPLS,
@@ -1634,14 +1634,10 @@ parse_mpls_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
         return ODP_FIT_TOO_LITTLE;
     }
     mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
-    if (mpls_lse == htonl(0)) {
-        /* Corner case for a truncated MPLS header. */
-        return fitness;
-    }
 
     /* Set mpls_lse. */
     flow->mpls_lse = mpls_lse;
-
+    flow->mpls_depth++;
 
     fitness = check_expectations(present_attrs, out_of_range_attr,
                                  expected_attrs, key, key_len);
@@ -2048,14 +2044,14 @@ static void
 commit_mpls_action(const struct flow *flow, struct flow *base,
                    struct ofpbuf *odp_actions)
 {
-    if (flow->mpls_lse == base->mpls_lse) {
+    if (flow->mpls_lse == base->mpls_lse &&
+        flow->mpls_depth == base->mpls_depth) {
         return;
     }
 
-    if (base->mpls_lse != htonl(0) && flow->mpls_lse == htonl(0)) {
-        nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_POP_MPLS,
-                        flow->dl_type);
-    } else if (base->mpls_lse == htonl(0) && flow->mpls_lse != htonl(0)) {
+    if (flow->mpls_depth < base->mpls_depth) {
+        nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_POP_MPLS, flow->dl_type);
+    } else if (flow->mpls_depth > base->mpls_depth) {
         struct ovs_action_push_mpls *mpls;
 
         mpls = nl_msg_put_unspec_uninit(odp_actions, OVS_ACTION_ATTR_PUSH_MPLS,
@@ -2069,6 +2065,7 @@ commit_mpls_action(const struct flow *flow, struct flow *base,
 
     base->dl_type = flow->dl_type;
     base->mpls_lse = flow->mpls_lse;
+    base->mpls_depth = flow->mpls_depth;
 }
 
 static void
diff --git a/lib/packets.c b/lib/packets.c
index 3f92b6a..e3182a1 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -193,7 +193,8 @@ eth_push_vlan(struct ofpbuf *packet, ovs_be16 tci)
 
 /* Removes outermost VLAN header (if any is present) from 'packet'.
  *
- * 'packet->l2' must initially point to 'packet''s Ethernet header. */
+ * 'packet->l2_5' should initially point to 'packet''s outer-most MPLS header
+ * or may be NULL if there are no MPLS headers. */
 void
 eth_pop_vlan(struct ofpbuf *packet)
 {
@@ -212,6 +213,28 @@ eth_pop_vlan(struct ofpbuf *packet)
     }
 }
 
+/* Return depth of mpls stack.
+ *
+ * 'packet->l2' must initially point to 'packet''s Ethernet header. */
+uint16_t
+eth_mpls_depth(struct ofpbuf *packet)
+{
+    struct mpls_hdr *mh = packet->l2_5;
+    uint16_t depth = 1;
+
+    if (!mh) {
+        return 0;
+    }
+
+    while (packet->size >= ((char *)mh - (char *)packet->data) + sizeof *mh &&
+           !(mh->mpls_lse & htonl(MPLS_BOS_MASK))) {
+        mh++;
+        depth++;
+    }
+
+    return depth;
+}
+
 /* Set ethertype of the packet. */
 void
 set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type)
diff --git a/lib/packets.h b/lib/packets.h
index 299da8f..4a5ef2a 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -140,6 +140,8 @@ void compose_rarp(struct ofpbuf *, const uint8_t eth_src[ETH_ADDR_LEN]);
 void eth_push_vlan(struct ofpbuf *, ovs_be16 tci);
 void eth_pop_vlan(struct ofpbuf *);
 
+uint16_t eth_mpls_depth(struct ofpbuf *packet);
+
 void set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type);
 
 const char *eth_from_hex(const char *hex, struct ofpbuf **packetp);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e5aa4e8..c9fb920 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4954,6 +4954,7 @@ compose_output_action__(struct action_xlate_ctx *ctx, uint16_t ofp_port,
     uint16_t odp_port = ofp_port_to_odp_port(ofp_port);
     ovs_be16 flow_vlan_tci = ctx->flow.vlan_tci;
     ovs_be32 flow_mpls_lse = ctx->flow.mpls_lse;
+    uint16_t flow_mpls_depth = ctx->flow.mpls_depth;
     uint8_t flow_nw_tos = ctx->flow.nw_tos;
     uint16_t out_port;
 
@@ -4992,6 +4993,7 @@ compose_output_action__(struct action_xlate_ctx *ctx, uint16_t ofp_port,
     ctx->nf_output_iface = ofp_port;
     ctx->flow.vlan_tci = flow_vlan_tci;
     ctx->flow.mpls_lse = flow_mpls_lse;
+    ctx->flow.mpls_depth = flow_mpls_depth;
     ctx->flow.nw_tos = flow_nw_tos;
 }
 
@@ -5133,6 +5135,7 @@ execute_controller_action(struct action_xlate_ctx *ctx, int len,
 
     if (packet->l2 && packet->l3) {
         struct eth_header *eh;
+        uint16_t mpls_depth;
 
         eth_pop_vlan(packet);
         eh = packet->l2;
@@ -5161,15 +5164,29 @@ execute_controller_action(struct action_xlate_ctx *ctx, int len,
             eth_push_vlan(packet, ctx->flow.vlan_tci);
         }
 
-        if (ctx->flow.mpls_lse) {
-            push_mpls(packet, ctx->flow.dl_type);
-            set_mpls_lse(packet, ctx->flow.mpls_lse);
+        mpls_depth = eth_mpls_depth(packet);
+        VLOG_WARN("execute_controller_action: mpls depth %p %u %u",
+                  packet->l2_5, mpls_depth, ctx->flow.mpls_depth);
+        if (packet->l2_5) {
+            struct mpls_hdr *mh = packet->l2_5;
+        VLOG_WARN("execute_controller_action: size %zu %zu",
+                  packet->size,
+                  ((char *)mh - (char *)packet->data) + sizeof *mh);
+        VLOG_WARN("execute_controller_action: bos %d",
+                  mh->mpls_lse & htonl(MPLS_BOS_MASK));
+
         }
 
-        if (!ctx->flow.mpls_lse &&
-            (eh->eth_type == htons(ETH_TYPE_MPLS) ||
-             eh->eth_type == htons(ETH_TYPE_MPLS_MCAST))) {
+        if (mpls_depth < ctx->flow.mpls_depth) {
+            push_mpls(packet, ctx->flow.dl_type);
+            set_mpls_lse(packet, ctx->flow.mpls_lse);
+            VLOG_WARN("execute_controller_action: push mpls");
+        } else if (mpls_depth > ctx->flow.mpls_depth) {
             pop_mpls(packet, ctx->flow.dl_type);
+            VLOG_WARN("execute_controller_action: pop mpls");
+        } else if (mpls_depth) {
+            set_mpls_lse(packet, ctx->flow.mpls_lse);
+            VLOG_WARN("execute_controller_action: set");
         }
 
         if (packet->l4) {
@@ -5210,9 +5227,10 @@ compose_mpls_push_action(struct action_xlate_ctx *ctx, ovs_be16 eth_type)
     assert(eth_type == htons(ETH_TYPE_MPLS) ||
            eth_type == htons(ETH_TYPE_MPLS_MCAST));
 
-    if (ctx->base_flow.mpls_lse != htonl(0)) {
+    if (ctx->base_flow.mpls_depth) {
         ctx->flow.mpls_lse = ctx->base_flow.mpls_lse;
         ctx->flow.mpls_lse &= ~htonl(MPLS_BOS_MASK);
+        ctx->flow.mpls_depth = 1;
     } else {
         ovs_be32 mpls_label, mpls_tc, mpls_ttl, mpls_bos;
 
@@ -5232,6 +5250,7 @@ compose_mpls_push_action(struct action_xlate_ctx *ctx, ovs_be16 eth_type)
         mpls_bos = htonl(0x1 << MPLS_BOS_SHIFT);
         ctx->flow.mpls_lse = mpls_label | mpls_tc | mpls_ttl | mpls_bos;
         ctx->flow.encap_dl_type = ctx->flow.dl_type;
+        ctx->flow.mpls_depth++;
     }
     ctx->flow.dl_type = eth_type;
 }
@@ -5246,11 +5265,12 @@ compose_mpls_pop_action(struct action_xlate_ctx *ctx, ovs_be16 eth_type)
 
     nl_msg_put_be16(ctx->odp_actions, OVS_ACTION_ATTR_POP_MPLS, eth_type);
  
-    if (ctx->flow.mpls_lse & htonl(MPLS_BOS_MASK)) {
+    if (ctx->flow.mpls_depth) {
         ctx->flow.dl_type = eth_type;
         ctx->flow.encap_dl_type = htons(0);
+        ctx->flow.mpls_depth--;
+        ctx->base_flow.mpls_depth--;
     }
-    ctx->base_flow.mpls_lse = htonl(0);
 }
 
 static bool
@@ -5640,7 +5660,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_POP_MPLS:
             compose_mpls_pop_action(ctx, ofpact_get_POP_MPLS(a)->ethertype);
-            ctx->flow.mpls_lse = htonl(0);
             break;
 
         case OFPACT_DEC_TTL:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e380c7e..d5aa0bd 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -342,13 +342,13 @@ done
 
 OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
 AT_CHECK([cat ofctl_monitor.log], [0], [dnl
-NXT_PACKET_IN (xid=0x0): cookie=0xb total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+NXT_PACKET_IN (xid=0x0): cookie=0xb total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
 priority:0,metadata:0,in_port:0000,tci(0) mac(50:55:55:55:55:55->50:54:00:00:00:07) type:8847,mpls(label:1000,tc:7,ttl:64,bos:1)
 dnl
-NXT_PACKET_IN (xid=0x0): cookie=0xb total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+NXT_PACKET_IN (xid=0x0): cookie=0xb total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
 priority:0,metadata:0,in_port:0000,tci(0) mac(50:55:55:55:55:55->50:54:00:00:00:07) type:8847,mpls(label:1000,tc:7,ttl:64,bos:1)
 dnl
-NXT_PACKET_IN (xid=0x0): cookie=0xb total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+NXT_PACKET_IN (xid=0x0): cookie=0xb total_len=60 in_port=1 (via action) data_len=60 (unbuffered)
 priority:0,metadata:0,in_port:0000,tci(0) mac(50:55:55:55:55:55->50:54:00:00:00:07) type:8847,mpls(label:1000,tc:7,ttl:64,bos:1)
 ])
 
diff --git a/tests/test-bundle.c b/tests/test-bundle.c
index aa8b6f0..f5b24b4 100644
--- a/tests/test-bundle.c
+++ b/tests/test-bundle.c
@@ -137,6 +137,7 @@ main(int argc, char *argv[])
     for (i = 0; i < N_FLOWS; i++) {
         random_bytes(&flows[i], sizeof flows[i]);
         memset(flows[i].zeros, 0, sizeof flows[i].zeros);
+        flows[i].mpls_depth = 0;
         flows[i].regs[0] = OFPP_NONE;
     }
 
diff --git a/tests/test-multipath.c b/tests/test-multipath.c
index b990c13..8442bc2 100644
--- a/tests/test-multipath.c
+++ b/tests/test-multipath.c
@@ -61,6 +61,7 @@ main(int argc, char *argv[])
 
             random_bytes(&flow, sizeof flow);
             memset(flow.zeros, 0, sizeof flow.zeros);
+            flow.mpls_depth = 0;
 
             mp.max_link = n - 1;
             multipath_execute(&mp, &flow);
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index d254d01..d9f8925 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -935,7 +935,7 @@ dpctl_normalize_actions(int argc, char *argv[])
             printf("no vlan: ");
         }
 
-        if (af->flow.mpls_lse != htonl(0)) {
+        if (af->flow.mpls_depth) {
             printf("mpls(label=%"PRIu32",tc=%d,ttl=%d): ",
                    mpls_lse_to_label(af->flow.mpls_lse),
                    mpls_lse_to_tc(af->flow.mpls_lse),
-- 
1.7.10.4




More information about the dev mailing list