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 <[email protected]>
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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
