Hi Ben, I've cloned https://github.com/horms/openvswitch.git but it doesn't contain the latest modification "for 3 mpls label support" , should I move to annother branch ( https://github.com/blp/ovs-reviews.gitmpls) to have the latest modification ?
Kind regards, Mounir On Thu, Jan 9, 2014 at 6:17 PM, Ben Pfaff <b...@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 <b...@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 <b...@nicira.com> > > > Co-authored-by: Simon Horman <ho...@verge.net.au> > > > Signed-off-by: Simon Horman <ho...@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) { > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
