[ovs-dev] [PATCH 35/41] ofproto-dpif-rid: Use separate pointers for actions and action set.
Jarno Rajahalme
jarno at ovn.org
Wed Jan 20 23:59:32 UTC 2016
Acked-by: Jarno Rajahalme <jarno at ovn.org>
> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <blp at ovn.org> wrote:
>
> During translation it makes some sense to concatenate these in a single
> array, but in my opinion it's conceptually better to separate them for
> the recirc_state; they are not naturally the same thing.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> ofproto/ofproto-dpif-rid.c | 21 +++++++++++++++++----
> ofproto/ofproto-dpif-rid.h | 8 ++++----
> ofproto/ofproto-dpif-xlate.c | 21 +++++++++++----------
> 3 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> index f4dc21f..f2d3c96 100644
> --- a/ofproto/ofproto-dpif-rid.c
> +++ b/ofproto/ofproto-dpif-rid.c
> @@ -148,6 +148,10 @@ recirc_metadata_hash(const struct recirc_state *state)
> }
> hash = hash_int(state->mirrors, hash);
> hash = hash_int(state->action_set_len, hash);
> + if (state->action_set_len) {
> + hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->action_set),
> + state->action_set_len, hash);
> + }
> if (state->ofpacts_len) {
> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
> state->ofpacts_len, hash);
> @@ -168,9 +172,10 @@ recirc_metadata_equal(const struct recirc_state *a,
> && !memcmp(a->stack, b->stack, a->n_stack * sizeof *a->stack)
> && a->mirrors == b->mirrors
> && a->conntracked == b->conntracked
> - && a->action_set_len == b->action_set_len
> && ofpacts_equal(a->ofpacts, a->ofpacts_len,
> - b->ofpacts, b->ofpacts_len));
> + b->ofpacts, b->ofpacts_len)
> + && ofpacts_equal(a->action_set, a->action_set_len,
> + b->action_set, b->action_set_len));
> }
>
> /* Lockless RCU protected lookup. If node is needed accross RCU quiescent
> @@ -210,14 +215,21 @@ recirc_state_clone(struct recirc_state *new, const struct recirc_state *old,
> flow_tnl_copy__(tunnel, old->metadata.tunnel);
> new->metadata.tunnel = tunnel;
>
> - if (new->n_stack) {
> - new->stack = xmemdup(new->stack, new->n_stack * sizeof *new->stack);
> + if (new->stack) {
> + new->stack = (new->n_stack
> + ? xmemdup(new->stack, new->n_stack * sizeof *new->stack)
> + : NULL);
> }
> if (new->ofpacts) {
> new->ofpacts = (new->ofpacts_len
> ? xmemdup(new->ofpacts, new->ofpacts_len)
> : NULL);
> }
> + if (new->action_set) {
> + new->action_set = (new->action_set_len
> + ? xmemdup(new->action_set, new->action_set_len)
> + : NULL);
> + }
> }
>
> static void
> @@ -225,6 +237,7 @@ recirc_state_free(struct recirc_state *state)
> {
> free(state->stack);
> free(state->ofpacts);
> + free(state->action_set);
> }
>
> /* Allocate a unique recirculation id for the given set of flow metadata.
> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> index 101bd6e..ba968ba 100644
> --- a/ofproto/ofproto-dpif-rid.h
> +++ b/ofproto/ofproto-dpif-rid.h
> @@ -146,10 +146,10 @@ struct recirc_state {
> bool conntracked; /* Conntrack occurred prior to recirc. */
>
> /* Actions to be translated on recirculation. */
> - uint32_t action_set_len; /* How much of 'ofpacts' consists of an
> - * action set? */
> - uint32_t ofpacts_len; /* Size of 'ofpacts', in bytes. */
> - struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */
> + struct ofpact *ofpacts;
> + size_t ofpacts_len; /* Size of 'ofpacts', in bytes. */
> + struct ofpact *action_set;
> + size_t action_set_len; /* Size of 'action_set', in bytes. */
> };
>
> /* This maps a recirculation ID to saved state that flow translation can
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 6d67e91..1140652 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3630,9 +3630,11 @@ 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,
> + .action_set = ctx->action_set.data,
> .action_set_len = ctx->recirc_action_offset,
> - .ofpacts_len = ctx->action_set.size,
> - .ofpacts = ctx->action_set.data,
> };
>
> /* Allocate a unique recirc id for the given metadata state in the
> @@ -5176,11 +5178,12 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> const struct ofpact *a;
>
> xlate_report_actions(&ctx, "- Restoring action set",
> - state->ofpacts, state->action_set_len);
> + state->action_set, state->action_set_len);
>
> - ofpbuf_put(&ctx.action_set, state->ofpacts, state->action_set_len);
> + ofpbuf_put(&ctx.action_set,
> + state->action_set, state->action_set_len);
>
> - OFPACT_FOR_EACH(a, state->ofpacts, state->action_set_len) {
> + OFPACT_FOR_EACH (a, state->action_set, state->action_set_len) {
> if (a->type == OFPACT_GROUP) {
> ctx.action_set_has_group = true;
> break;
> @@ -5190,11 +5193,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>
> /* Restore recirculation actions. If there are no actions, processing
> * will start with a lookup in the table set above. */
> - if (state->ofpacts_len > state->action_set_len) {
> - xin->ofpacts_len = state->ofpacts_len - state->action_set_len;
> - xin->ofpacts = state->ofpacts +
> - state->action_set_len / sizeof *state->ofpacts;
> -
> + xin->ofpacts = state->ofpacts;
> + xin->ofpacts_len = state->ofpacts_len;
> + if (state->ofpacts_len) {
> xlate_report_actions(&ctx, "- Restoring actions",
> xin->ofpacts, xin->ofpacts_len);
> }
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list