On Tue, Oct 16, 2012 at 07:23:32AM +0900, Isaku Yamahata wrote:
> On Mon, Oct 15, 2012 at 02:42:27PM +0900, Simon Horman wrote:
> > 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.
>
> Basically looks good. some comments inlined.
>
>
> > ----------------------------------------------------------------
> > Simon Horman <ho...@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,
>
> Minor nit pick. Right now only single lable push/pop is allowed, so how about
> assert(diff mpsl_depth <= 1)
I think an assert is rather strong.
As it is, a depth change of greater than one will be composed into a single
push or pop, which I think is satisfactory though possibly confusing.
I suggest adding a rate-limited warning if if we want to do anything about
this in the short term..
> > @@ -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));
> > +
>
> Indent.
Thanks!
> > }
> >
> > - 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;
> ctx->flow.mpls_depth++
> > } 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.mpls_depth = 1;
Oops, thanks.
> > }
> > 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);
> > }
>
> It looks somewhat confused. Probably something like
> the following pseudo code
>
> if (mpls_depth) {
> ctx->flow.mpls_depth--;
> ctx->base_flow.mpls_depth--;
> if (!mpls_depth) {
> ctx->flow.dl_type = eth_type;
> ctx->flow.encap_dl_type = htons(0);
> }
> }
Thanks, I have changed the code to the following:
if (ctx->flow.mpls_depth) {
ctx->flow.mpls_depth--;
ctx->base_flow.mpls_depth--;
if (!ctx->flow.mpls_depth) {
ctx->flow.dl_type = eth_type;
ctx->flow.encap_dl_type = htons(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
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
>
> --
> yamahata
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
