On Fri, Jan 17, 2014 at 01:01:29PM -0800, Ben Pfaff wrote:
> On Fri, Jan 17, 2014 at 11:33:45AM +0900, Simon Horman wrote:
> > On Thu, Jan 16, 2014 at 05:28:53PM -0800, Ben Pfaff wrote:
> > > On Fri, Jan 17, 2014 at 09:59:49AM +0900, Simon Horman wrote:
> > > > On Thu, Jan 16, 2014 at 04:46:23PM -0800, Ben Pfaff wrote:
> > > > > On Wed, Jan 15, 2014 at 04:13:25PM +0900, Simon Horman wrote:
> > > > > > The key is sourced from the datapath so should not
> > > > > > have more labels than it can handle.
> > > > > >
> > > > > > dpif-netdev supports as many LSEs as can fit in struct flow,
> > > > > > so it is safe to pass SIZE_MAX as the limit there.
> > > > > >
> > > > > > This is an proposed enhancement to
> > > > > > "Implement OpenFlow support for MPLS, for up to 3 labels."
> > > > > >
> > > > > > Signed-off-by: Simon Horman <ho...@verge.net.au>
> > > > >
> > > > > I don't think we'd ever try to install a flow that matches on more
> > > > > MPLS labels than we probed the datapath as supporting. If we do, that
> > > > > sounds like a bug. I think that this change would just make the bug
> > > > > harder to find, by making us suppress the error inside odp-util
> > > > > without actually fixing the bug at the higher layer.
> > > >
> > > > The problem I was trying to address is as follows:
> > > >
> > > > Suppose that the datapath supports one MPLS LSE and sends a that key
> > > > with
> > > > one MPLS LSE which does not have the BoS bit set. I believe that the
> > > > current code would create a mask with three LSEs masked instead of one.
> > > > But perhaps this is not a problem.
> > >
> > > I think that in that case the proper userspace behavior would be to
> > > parse the flow from the kernel as ODP_FIT_TOO_LITTLE, which means that
> > > userspace should never try to set up a flow. Oh, except that it would
> > > set up a slow-path flow. Ouch. Hmm.
> >
> > Just to clarify. I am talking about a situation where:
> > 1. The datapath supports fewer LSEs than ovs-vswitchd and;
> > 2. The packet has more LSEs than the datapath supports
>
> My message above was concluding that we do need to do something, and I
> ran out of time at that point. I'm going to reconsider the patch. I
> don't really like it at first glance, so I'm going to see if I can
> think of a way I like better.
Hi Ben,
I think that the key here is that when the translation is made from
key attributes to a struct flow some information may be is lost: the number
of MPLS LSE attributes that were present in the key.
I think it is reasonable for the datapath to send fewer attributes
than ovs-vswitchd in two cases.
1) If there were fewer LSEs present in the packet than ovs-vswitchd can handle.
I believe this already occurs when using the user-space datapath and
I believe that your change to make odp_flow_key_to_flow__() return
ODP_FIT_TOO_LITTLE in this case causes the test-suite to fail with
dpif_netdev_mask_from_nlattrs() reporting
"internal error parsing flow mask..."
I wonder if odp_flow_key_to_flow__() should ODP_FIT_PERFECT in this case.
2) In the case where it supports fewer LSEs than ovs-vswtichd.
This was the motivation for this patch.
I have tried coding up an alternate approach to addressing this,
passing a max_labels parameter to flow_count_mpls_labels();
passing the probed maximum number of labels supported to the datapath
to directly and indirectly via its callers; and limiting len
in flow_count_mpls_labels() accordingly.
The general idea is to limit the scope of setting of
wc->masks.mpls_lse.
This is a much more verbose change than the patch that started this
thread and as a result I am much less comfortable about its
correctness. But for reference it is below.
I think that a simpler but more hackish way to get the same effect would
be to zero the trailing portion of wc->masks.mpls_lse at
some point according the number of labels supported by the datapath.
Perhaps temporarily before calling odp_flow_key_from_mask().
From: Simon Horman <ho...@verge.net.au>
Limit MPLS labels scanned by flow_count_mpls_labels
Signed-off-by: Simon Horman <ho...@verge.net.au>
diff --git a/lib/flow.c b/lib/flow.c
index e3d4221..33121a9 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1054,14 +1054,15 @@ 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, struct flow_wildcards *wc)
+flow_count_mpls_labels(const struct flow *flow, struct flow_wildcards *wc,
+ size_t max_labels)
{
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);
+ int len = MIN(max_labels, ARRAY_SIZE(flow->mpls_lse));
for (i = 0; i < len; i++) {
if (wc) {
@@ -1084,10 +1085,11 @@ flow_count_mpls_labels(const struct flow *flow, struct
flow_wildcards *wc)
int
flow_count_common_mpls_labels(const struct flow *flow_a,
const struct flow *flow_b,
- struct flow_wildcards *wc)
+ struct flow_wildcards *wc,
+ size_t max_labels)
{
- int flow_a_n = flow_count_mpls_labels(flow_a, wc);
- int flow_b_n = flow_count_mpls_labels(flow_b, wc);
+ int flow_a_n = flow_count_mpls_labels(flow_a, wc, max_labels);
+ int flow_b_n = flow_count_mpls_labels(flow_b, wc, max_labels);
int min_n = MIN(flow_a_n, flow_b_n);
if (min_n == 0) {
@@ -1116,9 +1118,9 @@ flow_count_common_mpls_labels(const struct flow *flow_a,
void
flow_push_mpls(struct flow *flow, ovs_be16 mpls_eth_type,
- struct flow_wildcards *wc)
+ struct flow_wildcards *wc, size_t max_labels)
{
- int n = flow_count_mpls_labels(flow, wc);
+ int n = flow_count_mpls_labels(flow, wc, max_labels);
ovs_assert(n < ARRAY_SIZE(flow->mpls_lse));
@@ -1151,9 +1153,10 @@ 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)
+flow_pop_mpls(struct flow *flow, ovs_be16 eth_type, struct flow_wildcards *wc,
+ size_t max_labels)
{
- int n = flow_count_mpls_labels(flow, wc);
+ int n = flow_count_mpls_labels(flow, wc, max_labels);
int i;
if (n == 0) {
diff --git a/lib/flow.h b/lib/flow.h
index 69d406b..e7c10c5 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -195,13 +195,16 @@ void flow_set_dl_vlan(struct flow *, ovs_be16 vid);
void flow_set_vlan_vid(struct flow *, ovs_be16 vid);
void flow_set_vlan_pcp(struct flow *, uint8_t pcp);
-int flow_count_mpls_labels(const struct flow *, struct flow_wildcards *);
+int flow_count_mpls_labels(const struct flow *, struct flow_wildcards *,
+ size_t max_labels);
int flow_count_common_mpls_labels(const struct flow *flow_a,
const struct flow *flow_b,
- struct flow_wildcards *wc);
+ struct flow_wildcards *wc,
+ size_t max_labels);
void flow_push_mpls(struct flow *, ovs_be16 mpls_eth_type,
- struct flow_wildcards *);
-bool flow_pop_mpls(struct flow *, ovs_be16 eth_type, struct flow_wildcards *);
+ struct flow_wildcards *, size_t max_labels);
+bool flow_pop_mpls(struct flow *, ovs_be16 eth_type, struct flow_wildcards *,
+ size_t max_labels);
void flow_set_mpls_label(struct flow *, int idx, ovs_be32 label);
void flow_set_mpls_ttl(struct flow *, int idx, uint8_t ttl);
void flow_set_mpls_tc(struct flow *, int idx, uint8_t tc);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 520b314..c38f1be 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2523,7 +2523,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const struct
flow *data,
struct ovs_key_mpls *mpls_key;
int i, n;
- n = flow_count_mpls_labels(flow, NULL);
+ n = flow_count_mpls_labels(flow, NULL, SIZE_MAX);
mpls_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_MPLS,
n * sizeof *mpls_key);
for (i = 0; i < n; i++) {
@@ -3475,11 +3475,12 @@ commit_vlan_action(ovs_be16 vlan_tci, struct flow *base,
static void
commit_mpls_action(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+ size_t max_labels)
{
- int base_n = flow_count_mpls_labels(base, wc);
- int flow_n = flow_count_mpls_labels(flow, wc);
- int common_n = flow_count_common_mpls_labels(flow, base, wc);
+ int base_n = flow_count_mpls_labels(base, wc, max_labels);
+ int flow_n = flow_count_mpls_labels(flow, wc, max_labels);
+ int common_n = flow_count_common_mpls_labels(flow, base, wc, max_labels);
while (base_n > common_n) {
if (base_n - 1 == common_n && flow_n > common_n) {
@@ -3516,7 +3517,7 @@ commit_mpls_action(const struct flow *flow, struct flow
*base,
dl_type = flow->dl_type;
}
nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_POP_MPLS, dl_type);
- popped = flow_pop_mpls(base, flow->dl_type, wc);
+ popped = flow_pop_mpls(base, flow->dl_type, wc, max_labels);
ovs_assert(popped);
base_n--;
}
@@ -3536,7 +3537,7 @@ commit_mpls_action(const struct flow *flow, struct flow
*base,
sizeof *mpls);
mpls->mpls_ethertype = flow->dl_type;
mpls->mpls_lse = flow->mpls_lse[flow_n - base_n - 1];
- flow_push_mpls(base, mpls->mpls_ethertype, wc);
+ flow_push_mpls(base, mpls->mpls_ethertype, wc, max_labels);
flow_set_mpls_lse(base, 0, mpls->mpls_lse);
base_n++;
}
@@ -3760,14 +3761,15 @@ commit_set_pkt_mark_action(const struct flow *flow,
struct flow *base,
* slow path, if there is one, otherwise 0. */
enum slow_path_reason
commit_odp_actions(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+ size_t max_mpls_labels)
{
enum slow_path_reason slow;
commit_set_ether_addr_action(flow, base, odp_actions, wc);
slow = commit_set_nw_action(flow, base, odp_actions, wc);
commit_set_port_action(flow, base, odp_actions, wc);
- commit_mpls_action(flow, base, odp_actions, wc);
+ commit_mpls_action(flow, base, odp_actions, wc, max_mpls_labels);
commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
commit_set_priority_action(flow, base, odp_actions, wc);
commit_set_pkt_mark_action(flow, base, odp_actions, wc);
diff --git a/lib/odp-util.h b/lib/odp-util.h
index abb8789..60f8871 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -181,7 +181,8 @@ void commit_odp_tunnel_action(const struct flow *, struct
flow *base,
enum slow_path_reason commit_odp_actions(const struct flow *,
struct flow *base,
struct ofpbuf *odp_actions,
- struct flow_wildcards *wc);
+ struct flow_wildcards *wc,
+ size_t max_mpls_labels);
/* ofproto-dpif interface.
*
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f067456..b98e519 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1799,7 +1799,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t
ofp_port,
if (out_port != ODPP_NONE) {
ctx->xout->slow |= commit_odp_actions(flow, &ctx->base_flow,
&ctx->xout->odp_actions,
- &ctx->xout->wc);
+ &ctx->xout->wc,
+ ctx->xbridge->max_mpls_depth);
nl_msg_put_odp_port(&ctx->xout->odp_actions, OVS_ACTION_ATTR_OUTPUT,
out_port);
@@ -2074,7 +2075,8 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
&ctx->xout->odp_actions,
- &ctx->xout->wc);
+ &ctx->xout->wc,
+ ctx->xbridge->max_mpls_depth);
odp_execute_actions(NULL, packet, &md, ctx->xout->odp_actions.data,
ctx->xout->odp_actions.size, NULL);
@@ -2108,7 +2110,7 @@ compose_mpls_push_action(struct xlate_ctx *ctx, struct
ofpact_push_mpls *mpls)
ovs_assert(eth_type_mpls(mpls->ethertype));
- n = flow_count_mpls_labels(flow, wc);
+ n = flow_count_mpls_labels(flow, wc, ctx->xbridge->max_mpls_depth);
if (!n) {
if (mpls->position == OFPACT_MPLS_BEFORE_VLAN) {
vlan_tci = 0;
@@ -2117,14 +2119,15 @@ compose_mpls_push_action(struct xlate_ctx *ctx, struct
ofpact_push_mpls *mpls)
}
ctx->xout->slow |= commit_odp_actions(flow, &ctx->base_flow,
&ctx->xout->odp_actions,
- &ctx->xout->wc);
+ &ctx->xout->wc,
+ ctx->xbridge->max_mpls_depth);
} else if (n >= ctx->xbridge->max_mpls_depth) {
COVERAGE_INC(xlate_actions_mpls_overflow);
ctx->xout->slow |= SLOW_ACTION;
return;
}
- flow_push_mpls(flow, mpls->ethertype, wc);
+ flow_push_mpls(flow, mpls->ethertype, wc, ctx->xbridge->max_mpls_depth);
flow->vlan_tci = vlan_tci;
}
@@ -2134,8 +2137,9 @@ compose_mpls_pop_action(struct xlate_ctx *ctx, ovs_be16
eth_type)
struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
- if (!flow_pop_mpls(flow, eth_type, wc)
- && flow_count_mpls_labels(flow, wc) >= ARRAY_SIZE(flow->mpls_lse)) {
+ if (!flow_pop_mpls(flow, eth_type, wc, ctx->xbridge->max_mpls_depth)
+ && flow_count_mpls_labels(flow, wc, ctx->xbridge->max_mpls_depth) >=
+ ARRAY_SIZE(flow->mpls_lse)) {
if (ctx->xin->packet != NULL) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "bridge %s: dropping packet on which an "
@@ -2433,7 +2437,8 @@ xlate_sample_action(struct xlate_ctx *ctx,
ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
&ctx->xout->odp_actions,
- &ctx->xout->wc);
+ &ctx->xout->wc,
+ ctx->xbridge->max_mpls_depth);
compose_flow_sample_cookie(os->probability, os->collector_set_id,
os->obs_domain_id, os->obs_point_id, &cookie);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
