Until now, the flow translation code has tried to avoid constructing a set of wildcards during translation in the cases where it can, because wildcards are large and somewhat expensive. However, this has problems that we hadn't previously realized. Specifically, the generated actions can depend on the constructed wildcards, to decide which bits of a field need to be set in a masked set_field action. This means that in practice translation needs to always construct the wildcards.
(It might be possible to avoid masked set_field when we're not constructing wildcards, but this would mean that we'd generate different actions depending on whether wildcards were being constructed, which seems rather confusing at best. Also, the cases in which we don't need wildcards anyway are fairly obscure, meaning that the benefits of avoiding them in those cases are minimal and that it's going to be hard to get test coverage. The latter is probably why we didn't notice this until now.) Reported-by: William Tu <u9012...@gmail.com> Reported-at: http://openvswitch.org/pipermail/dev/2016-April/069219.html Signed-off-by: Ben Pfaff <b...@ovn.org> --- ofproto/ofproto-dpif-xlate.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 5937913..def0f7f 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -183,9 +183,7 @@ struct xlate_ctx { /* Flow translation populates this with wildcards relevant in translation. * When 'xin->wc' is nonnull, this is the same pointer. When 'xin->wc' is - * null, this is a pointer to uninitialized scratch memory. This allows - * code to blindly write to 'ctx->wc' without worrying about whether the - * caller really wants wildcards. */ + * null, this is a pointer to a temporary buffer. */ struct flow_wildcards *wc; /* Output buffer for datapath actions. When 'xin->odp_actions' is nonnull, @@ -3268,7 +3266,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, rule = rule_dpif_lookup_from_table(ctx->xbridge->ofproto, ctx->tables_version, - &ctx->xin->flow, ctx->xin->wc, + &ctx->xin->flow, ctx->wc, ctx->xin->resubmit_stats, &ctx->table_id, in_port, may_packet_in, honor_table_miss); @@ -5075,7 +5073,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) union mf_subvalue stack_stub[1024 / sizeof(union mf_subvalue)]; uint64_t action_set_stub[1024 / 8]; uint64_t frozen_actions_stub[1024 / 8]; - struct flow_wildcards scratch_wc; uint64_t actions_stub[256 / 8]; struct ofpbuf scratch_actions = OFPBUF_STUB_INITIALIZER(actions_stub); struct xlate_ctx ctx = { @@ -5086,7 +5083,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) .xbridge = xbridge, .stack = OFPBUF_STUB_INITIALIZER(stack_stub), .rule = xin->rule, - .wc = xin->wc ? xin->wc : &scratch_wc, + .wc = (xin->wc + ? xin->wc + : &(struct flow_wildcards) { .masks.dl_type = 0 }), .odp_actions = xin->odp_actions ? xin->odp_actions : &scratch_actions, .recurse = xin->recurse, @@ -5139,9 +5138,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) } ofpbuf_reserve(ctx.odp_actions, NL_A_U32_SIZE); - if (xin->wc) { - xlate_wc_init(&ctx); - } + xlate_wc_init(&ctx); COVERAGE_INC(xlate_actions); @@ -5231,7 +5228,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) if (!xin->ofpacts && !ctx.rule) { ctx.rule = rule_dpif_lookup_from_table( - ctx.xbridge->ofproto, ctx.tables_version, flow, xin->wc, + ctx.xbridge->ofproto, ctx.tables_version, flow, ctx.wc, ctx.xin->resubmit_stats, &ctx.table_id, flow->in_port.ofp_port, true, true); if (ctx.xin->resubmit_stats) { @@ -5382,9 +5379,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) } } - if (xin->wc) { - xlate_wc_finish(&ctx); - } + xlate_wc_finish(&ctx); exit: ofpbuf_uninit(&ctx.stack); -- 2.1.3 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
