With one question below:
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Feb 9, 2016, at 10:10 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> In my opinion, this is less confusing in multiple ways. I now understand
> the code better myself.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> ofproto/ofproto-dpif-xlate.c | 128 +++++++++++++++++--------------------------
> 1 file changed, 51 insertions(+), 77 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index aa10217..f48c5ac 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -258,31 +258,28 @@ struct xlate_ctx {
> * members. When a need for recirculation is identified, the translation
> * process:
> *
> - * 1. Sets 'recirc_action_offset' to the current size of 'action_set'.
> The
> - * action set is part of what needs to be preserved, so this allows the
> - * action set and the additional state to share the 'action_set'
> buffer.
> - * Later steps can tell that setup for recirculation is in progress
> from
> - * the nonnegative value of 'recirc_action_offset'.
> + * 1. Sets 'recirculating' to true.
> *
> * 2. Sets 'exit' to true to tell later steps that we're exiting from the
> * translation process.
> *
> - * 3. Adds an OFPACT_UNROLL_XLATE action to 'action_set'. This action
> - * holds the current table ID and cookie so that they can be restored
> - * during a post-recirculation upcall translation.
> + * 3. Adds an OFPACT_UNROLL_XLATE action to 'recirculate_actions', and
> + * points recirculate_actions.header to the action to make it easy to
> + * find it later. This action holds the current table ID and cookie so
> + * that they can be restored during a post-recirculation upcall
> + * translation.
> *
> * 4. Adds the action that prompted recirculation and any actions following
> - * it within the same flow to 'action_set', so that they can be
> executed
> - * during a post-recirculation upcall translation.
> + * it within the same flow to 'recirculate_actions', so that they can
> be
> + * executed during a post-recirculation upcall translation.
> *
> * 5. Returns.
> *
> * 6. The action that prompted recirculation might be nested in a stack of
> * nested "resubmit"s that have actions remaining. Each of these
> notices
> - * that we're exiting (from 'exit') and that recirculation setup is in
> - * progress (from 'recirc_action_offset') and responds by adding more
> - * OFPACT_UNROLL_XLATE actions to 'action_set', as necessary, and any
> - * actions that were yet unprocessed.
> + * that we're exiting and recirculating and responds by adding more
> + * OFPACT_UNROLL_XLATE actions to 'recirculate_actions', as necessary,
> + * and any actions that were yet unprocessed.
> *
> * The caller stores all the state produced by this process associated with
> * the recirculation ID. For post-recirculation upcall translation, the
> @@ -290,10 +287,8 @@ struct xlate_ctx {
> * process yielded a set of ofpacts that can be translated directly, so it
> * is not much of a special case at that point.
> */
> - int recirc_action_offset; /* Offset in 'action_set' to actions to be
> - * executed after recirculation, or -1. */
> - int last_unroll_offset; /* Offset in 'action_set' to the latest
> unroll
> - * action, or -1. */
> + bool recirculating;
> + struct ofpbuf recirculate_actions;
>
> /* True if a packet was but is no longer MPLS (due to an MPLS pop action).
> * This is a trigger for recirculation in cases where translating an
> action
> @@ -351,30 +346,22 @@ static void
> ctx_trigger_recirculation(struct xlate_ctx *ctx)
> {
> ctx->exit = true;
> - ctx->recirc_action_offset = ctx->action_set.size;
> + ctx->recirculating = true;
> }
>
> static bool
> ctx_first_recirculation_action(const struct xlate_ctx *ctx)
> {
> - return ctx->recirc_action_offset == ctx->action_set.size;
> -}
> -
> -static inline bool
> -exit_recirculates(const struct xlate_ctx *ctx)
> -{
> - /* When recirculating the 'recirc_action_offset' has a non-negative
> value.
> - */
> - return ctx->recirc_action_offset >= 0;
> + return !ctx->recirculate_actions.size;
> }
>
> static void
> ctx_cancel_recirculation(struct xlate_ctx *ctx)
> {
> - if (exit_recirculates(ctx)) {
> - ctx->action_set.size = ctx->recirc_action_offset;
> - ctx->recirc_action_offset = -1;
> - ctx->last_unroll_offset = -1;
> + if (ctx->recirculating) {
> + ctx->recirculating = false;
> + ofpbuf_clear(&ctx->recirculate_actions);
> + ctx->recirculate_actions.header = NULL;
> }
> }
>
> @@ -2995,15 +2982,10 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> if (!process_special(ctx, peer) && may_receive(peer, ctx)) {
> if (xport_stp_forward_state(peer) &&
> xport_rstp_forward_state(peer)) {
> xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
> true);
> - if (ctx->action_set.size) {
> - /* Translate action set only if not dropping the packet
> and
> - * not recirculating. */
> - if (!exit_recirculates(ctx)) {
> - xlate_action_set(ctx);
> - }
> + if (!ctx->recirculating) {
> + xlate_action_set(ctx);
> }
> - /* Check if need to recirculate. */
> - if (exit_recirculates(ctx)) {
> + if (ctx->recirculating) {
> compose_recirculate_action(ctx);
> }
> } else {
> @@ -3333,7 +3315,7 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct
> ofputil_bucket *bucket)
> ofpbuf_uninit(&action_list);
>
> /* Check if need to recirculate. */
> - if (exit_recirculates(ctx)) {
> + if (ctx->recirculating) {
> compose_recirculate_action(ctx);
> }
>
> @@ -3641,7 +3623,7 @@ compose_recirculate_action__(struct xlate_ctx *ctx,
> uint8_t table)
>
> recirc_metadata_from_flow(&md, &ctx->xin->flow);
>
> - ovs_assert(ctx->recirc_action_offset >= 0);
> + ovs_assert(ctx->recirculating);
>
> struct recirc_state state = {
> .table_id = table,
> @@ -3651,11 +3633,10 @@ compose_recirculate_action__(struct xlate_ctx *ctx,
> uint8_t table)
> .n_stack = ctx->stack.size / sizeof(union mf_subvalue),
> .mirrors = ctx->mirrors,
> .conntracked = ctx->conntracked,
> - .ofpacts = ((struct ofpact *) ctx->action_set.data
> - + ctx->recirc_action_offset / sizeof(struct ofpact)),
> - .ofpacts_len = ctx->action_set.size - ctx->recirc_action_offset,
> + .ofpacts = ctx->recirculate_actions.data,
> + .ofpacts_len = ctx->recirculate_actions.size,
> .action_set = ctx->action_set.data,
> - .action_set_len = ctx->recirc_action_offset,
> + .action_set_len = ctx->action_set.size,
> };
>
> /* Allocate a unique recirc id for the given metadata state in the
> @@ -3677,7 +3658,7 @@ compose_recirculate_action__(struct xlate_ctx *ctx,
> uint8_t table)
> ctx_cancel_recirculation(ctx);
> }
>
> -/* Called only when ctx->recirc_action_offset is set. */
> +/* Called only when we're recirculating. */
> static void
> compose_recirculate_action(struct xlate_ctx *ctx)
> {
> @@ -3692,7 +3673,7 @@ compose_recirculate_action(struct xlate_ctx *ctx)
> static void
> compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table)
> {
> - ctx->recirc_action_offset = ctx->action_set.size;
> + ctx->recirculating = true;
> compose_recirculate_action__(ctx, table);
> }
>
> @@ -4146,30 +4127,25 @@ xlate_action_set(struct xlate_ctx *ctx)
> static void
> recirc_put_unroll_xlate(struct xlate_ctx *ctx)
> {
> - struct ofpact_unroll_xlate *unroll;
> -
> - unroll = ctx->last_unroll_offset < 0
> - ? NULL
> - : ALIGNED_CAST(struct ofpact_unroll_xlate *,
> - (char *)ctx->action_set.data +
> ctx->last_unroll_offset);
> + struct ofpact_unroll_xlate *unroll = ctx->recirculate_actions.header;
>
> /* Restore the table_id and rule cookie for a potential PACKET
> * IN if needed. */
> if (!unroll ||
> (ctx->table_id != unroll->rule_table_id
> || ctx->rule_cookie != unroll->rule_cookie)) {
> -
> - ctx->last_unroll_offset = ctx->action_set.size;
> - unroll = ofpact_put_UNROLL_XLATE(&ctx->action_set);
> + unroll = ofpact_put_UNROLL_XLATE(&ctx->recirculate_actions);
> unroll->rule_table_id = ctx->table_id;
> unroll->rule_cookie = ctx->rule_cookie;
> + ctx->recirculate_actions.header = unroll;
> }
> }
>
>
> -/* Copy actions 'a' through 'end' to the action_set to be executed after
> - * recirculation. UNROLL_XLATE action is inserted, if not already done so,
> - * before actions that may depend on the current table ID or flow cookie. */
> +/* Copy actions 'a' through 'end' to ctx->recirculate_actions, which will be
> + * executed after recirculation. UNROLL_XLATE action is inserted, if not
> + * already done so, before actions that may depend on the current table ID or
> + * flow cookie. */
> static void
> recirc_unroll_actions(const struct ofpact *a, const struct ofpact *end,
> struct xlate_ctx *ctx)
> @@ -4245,7 +4221,7 @@ recirc_unroll_actions(const struct ofpact *a, const
> struct ofpact *end,
> continue;
> }
> /* Copy the action over. */
> - ofpbuf_put(&ctx->action_set, a, OFPACT_ALIGN(a->len));
> + ofpbuf_put(&ctx->recirculate_actions, a, OFPACT_ALIGN(a->len));
> }
> }
>
> @@ -4440,7 +4416,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> if (ctx->exit) {
> /* Check if need to store the remaining actions for later
> * execution. */
> - if (exit_recirculates(ctx)) {
> + if (ctx->recirculating) {
> recirc_unroll_actions(a, ofpact_end(ofpacts, ofpacts_len),
> ctx);
> }
> @@ -5094,6 +5070,7 @@ 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 recirculate_actions_stub[1024 / 8];
> struct flow_wildcards scratch_wc;
> uint64_t actions_stub[256 / 8];
> struct ofpbuf scratch_actions = OFPBUF_STUB_INITIALIZER(actions_stub);
> @@ -5123,8 +5100,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> .error = XLATE_OK,
> .mirrors = 0,
>
> - .recirc_action_offset = -1,
> - .last_unroll_offset = -1,
> + .recirculating = false,
> + .recirculate_actions =
> OFPBUF_STUB_INITIALIZER(recirculate_actions_stub),
>
> .was_mpls = false,
> .conntracked = false,
> @@ -5339,29 +5316,25 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> }
>
> /* We've let OFPP_NORMAL and the learning action look at the
> - * packet, so drop it now if forwarding is disabled. */
> + * packet, so cancel all actions and recirculation if forwarding
> is
> + * disabled. */
> if (in_port && (!xport_stp_forward_state(in_port) ||
> !xport_rstp_forward_state(in_port))) {
> - /* Drop all actions added by do_xlate_actions() above. */
> ctx.odp_actions->size = sample_actions_len;
> -
> - /* Undo changes that may have been done for recirculation. */
> ctx_cancel_recirculation(&ctx);
> - } else if (ctx.action_set.size) {
> - /* Translate action set only if not dropping the packet and
> - * not recirculating. */
> - if (!exit_recirculates(&ctx)) {
> - xlate_action_set(&ctx);
> - }
> + ofpbuf_clear(&ctx.action_set);
How come we did not do this before?
> + }
> +
> + if (!ctx.recirculating) {
> + xlate_action_set(&ctx);
> }
> - /* Check if need to recirculate. */
> - if (exit_recirculates(&ctx)) {
> + if (ctx.recirculating) {
> compose_recirculate_action(&ctx);
> }
> }
>
> /* Output only fully processed packets. */
> - if (!exit_recirculates(&ctx)
> + if (!ctx.recirculating
> && xbridge->has_in_band
> && in_band_must_output_to_local_port(flow)
> && !actions_output_to_local_port(&ctx)) {
> @@ -5411,6 +5384,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> exit:
> ofpbuf_uninit(&ctx.stack);
> ofpbuf_uninit(&ctx.action_set);
> + ofpbuf_uninit(&ctx.recirculate_actions);
> ofpbuf_uninit(&scratch_actions);
>
> /* Make sure we return a "drop flow" in case of an error. */
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
