[ovs-dev] [PATCH 23/26] ofproto-dpif-rid: Factor recirculation state out as new structure.
Jarno Rajahalme
jrajahalme at nicira.com
Fri Jul 31 21:54:54 UTC 2015
It’s almost like having named function arguments in C!
Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
> On Jul 29, 2015, at 11:42 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> This greatly reduces the number of arguments to many of the functions
> involved in recirculation, which to my eye makes the code clearer. It
> will also make it easier to add new recirculation state in an upcoming
> commit.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> ofproto/ofproto-dpif-rid.c | 162 ++++++++++++++++---------------------------
> ofproto/ofproto-dpif-rid.h | 43 +++++++-----
> ofproto/ofproto-dpif-xlate.c | 57 ++++++++-------
> 3 files changed, 117 insertions(+), 145 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> index f1b3bdc..e2867b7 100644
> --- a/ofproto/ofproto-dpif-rid.c
> +++ b/ofproto/ofproto-dpif-rid.c
> @@ -124,62 +124,52 @@ recirc_id_node_find(uint32_t id)
> }
>
> static uint32_t
> -recirc_metadata_hash(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts)
> +recirc_metadata_hash(const struct recirc_state *state)
> {
> uint32_t hash;
>
> - BUILD_ASSERT(OFPACT_ALIGNTO == sizeof(uint64_t));
> -
> - hash = hash_pointer(ofproto, 0);
> - hash = hash_int(table_id, hash);
> - hash = hash_words64((const uint64_t *)md, sizeof *md / sizeof(uint64_t),
> + hash = hash_pointer(state->ofproto, 0);
> + hash = hash_int(state->table_id, hash);
> + hash = hash_words64((const uint64_t *) &state->metadata,
> + sizeof state->metadata / sizeof(uint64_t),
> hash);
> - if (stack && stack->size != 0) {
> - hash = hash_words64((const uint64_t *)stack->data,
> - stack->size / sizeof(uint64_t), hash);
> + if (state->stack && state->stack->size != 0) {
> + hash = hash_words64((const uint64_t *) state->stack->data,
> + state->stack->size / sizeof(uint64_t), hash);
> }
> - hash = hash_int(action_set_len, hash);
> - if (ofpacts_len) {
> - hash = hash_words64(ALIGNED_CAST(const uint64_t *, ofpacts),
> - OFPACT_ALIGN(ofpacts_len) / sizeof(uint64_t),
> + hash = hash_int(state->action_set_len, hash);
> + if (state->ofpacts_len) {
> + hash = hash_words64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
> + state->ofpacts_len / sizeof(uint64_t),
> hash);
> }
> return hash;
> }
>
> static bool
> -recirc_metadata_equal(const struct recirc_id_node *node,
> - struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts)
> +recirc_metadata_equal(const struct recirc_state *a,
> + const struct recirc_state *b)
> {
> - return node->ofproto == ofproto
> - && node->table_id == table_id
> - && !memcmp(&node->metadata, md, sizeof *md)
> - && ((!node->stack && (!stack || stack->size == 0))
> - || (node->stack && stack && ofpbuf_equal(node->stack, stack)))
> - && node->action_set_len == action_set_len
> - && node->ofpacts_len == ofpacts_len
> - && (ofpacts_len == 0 || !memcmp(node->ofpacts, ofpacts, ofpacts_len));
> + return (a->table_id == b->table_id
> + && a->ofproto == b->ofproto
> + && !memcmp(&a->metadata, &b->metadata, sizeof a->metadata)
> + && (((!a->stack || !a->stack->size) &&
> + (!b->stack || !b->stack->size))
> + || (a->stack && b->stack && ofpbuf_equal(a->stack, b->stack)))
> + && a->action_set_len == b->action_set_len
> + && ofpacts_equal(a->ofpacts, a->ofpacts_len,
> + b->ofpacts, b->ofpacts_len));
> }
>
> /* Lockless RCU protected lookup. If node is needed accross RCU quiescent
> * state, caller should take a reference. */
> static struct recirc_id_node *
> -recirc_find_equal(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts, uint32_t hash)
> +recirc_find_equal(const struct recirc_state *target, uint32_t hash)
> {
> struct recirc_id_node *node;
>
> - CMAP_FOR_EACH_WITH_HASH(node, metadata_node, hash, &metadata_map) {
> - if (recirc_metadata_equal(node, ofproto, table_id, md, stack,
> - action_set_len, ofpacts_len, ofpacts)) {
> + CMAP_FOR_EACH_WITH_HASH (node, metadata_node, hash, &metadata_map) {
> + if (recirc_metadata_equal(&node->state, target)) {
> return node;
> }
> }
> @@ -187,16 +177,12 @@ recirc_find_equal(struct ofproto_dpif *ofproto, uint8_t table_id,
> }
>
> static struct recirc_id_node *
> -recirc_ref_equal(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts, uint32_t hash)
> +recirc_ref_equal(const struct recirc_state *target, uint32_t hash)
> {
> struct recirc_id_node *node;
>
> do {
> - node = recirc_find_equal(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts, hash);
> + node = recirc_find_equal(target, hash);
>
> /* Try again if the node was released before we get the reference. */
> } while (node && !ovs_refcount_try_ref_rcu(&node->refcount));
> @@ -204,30 +190,33 @@ recirc_ref_equal(struct ofproto_dpif *ofproto, uint8_t table_id,
> return node;
> }
>
> +static void
> +recirc_state_clone(struct recirc_state *new, const struct recirc_state *old)
> +{
> + *new = *old;
> + if (new->stack) {
> + new->stack = new->stack->size ? ofpbuf_clone(new->stack) : NULL;
> + }
> + if (new->ofpacts) {
> + new->ofpacts = (new->ofpacts_len
> + ? xmemdup(new->ofpacts, new->ofpacts_len)
> + : NULL);
> + }
> +}
> +
> /* Allocate a unique recirculation id for the given set of flow metadata.
> * The ID space is 2^^32, so there should never be a situation in which all
> * the IDs are used up. We loop until we find a free one.
> * hash is recomputed if it is passed in as 0. */
> static struct recirc_id_node *
> -recirc_alloc_id__(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts, uint32_t hash)
> +recirc_alloc_id__(const struct recirc_state *state, uint32_t hash)
> {
> - struct recirc_id_node *node = xzalloc(sizeof *node +
> - OFPACT_ALIGN(ofpacts_len));
> + ovs_assert(state->action_set_len <= state->ofpacts_len);
> +
> + struct recirc_id_node *node = xzalloc(sizeof *node);
> node->hash = hash;
> ovs_refcount_init(&node->refcount);
> -
> - node->ofproto = ofproto;
> - node->table_id = table_id;
> - memcpy(&node->metadata, md, sizeof node->metadata);
> - node->stack = (stack && stack->size) ? ofpbuf_clone(stack) : NULL;
> - node->action_set_len = action_set_len;
> - node->ofpacts_len = ofpacts_len;
> - if (ofpacts_len) {
> - memcpy(node->ofpacts, ofpacts, ofpacts_len);
> - }
> + recirc_state_clone(CONST_CAST(struct recirc_state *, &node->state), state);
>
> ovs_mutex_lock(&mutex);
> for (;;) {
> @@ -254,48 +243,23 @@ recirc_alloc_id__(struct ofproto_dpif *ofproto, uint8_t table_id,
> /* Look up an existing ID for the given flow's metadata and optional actions.
> */
> uint32_t
> -recirc_find_id(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts)
> +recirc_find_id(const struct recirc_state *target)
> {
> - /* Check if an ID with the given metadata already exists. */
> - struct recirc_id_node *node;
> - uint32_t hash;
> -
> - hash = recirc_metadata_hash(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts);
> - node = recirc_find_equal(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts, hash);
> -
> + uint32_t hash = recirc_metadata_hash(target);
> + struct recirc_id_node *node = recirc_find_equal(target, hash);
> return node ? node->id : 0;
> }
>
> /* Allocate a unique recirculation id for the given set of flow metadata and
> optional actions. */
> uint32_t
> -recirc_alloc_id_ctx(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts)
> +recirc_alloc_id_ctx(const struct recirc_state *state)
> {
> - struct recirc_id_node *node;
> - uint32_t hash;
> -
> - /* Look up an existing ID. */
> - hash = recirc_metadata_hash(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts);
> - node = recirc_ref_equal(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts, hash);
> -
> - /* Allocate a new recirc ID if needed. */
> + uint32_t hash = recirc_metadata_hash(state);
> + struct recirc_id_node *node = recirc_ref_equal(state, hash);
> if (!node) {
> - ovs_assert(action_set_len <= ofpacts_len);
> -
> - node = recirc_alloc_id__(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts, hash);
> + node = recirc_alloc_id__(state, hash);
> }
> -
> return node->id;
> }
>
> @@ -303,16 +267,12 @@ recirc_alloc_id_ctx(struct ofproto_dpif *ofproto, uint8_t table_id,
> uint32_t
> recirc_alloc_id(struct ofproto_dpif *ofproto)
> {
> - struct recirc_metadata md;
> - struct recirc_id_node *node;
> - uint32_t hash;
> -
> - memset(&md, 0, sizeof md);
> - md.in_port = OFPP_NONE;
> - hash = recirc_metadata_hash(ofproto, TBL_INTERNAL, &md, NULL, 0, 0, NULL);
> - node = recirc_alloc_id__(ofproto, TBL_INTERNAL, &md, NULL, 0, 0, NULL,
> - hash);
> - return node->id;
> + struct recirc_state state = {
> + .table_id = TBL_INTERNAL,
> + .ofproto = ofproto,
> + .metadata = { .in_port = OFPP_NONE },
> + };
> + return recirc_alloc_id__(&state, recirc_metadata_hash(&state))->id;
> }
>
> void
> @@ -356,7 +316,7 @@ recirc_free_ofproto(struct ofproto_dpif *ofproto, const char *ofproto_name)
> struct recirc_id_node *n;
>
> CMAP_FOR_EACH (n, metadata_node, &metadata_map) {
> - if (n->ofproto == ofproto) {
> + if (n->state.ofproto == ofproto) {
> VLOG_ERR("recirc_id %"PRIu32
> " left allocated when ofproto (%s)"
> " is destructed", n->id, ofproto_name);
> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> index a614a4d..9caa662 100644
> --- a/ofproto/ofproto-dpif-rid.h
> +++ b/ofproto/ofproto-dpif-rid.h
> @@ -125,16 +125,9 @@ recirc_metadata_to_flow(const struct recirc_metadata *md,
> flow->actset_output = md->actset_output;
> }
>
> -/* Pool node fields should NOT be modified after placing the node in the pool.
> - */
> -struct recirc_id_node {
> - struct ovs_list exp_node OVS_GUARDED;
> - struct cmap_node id_node;
> - struct cmap_node metadata_node;
> - uint32_t id;
> - uint32_t hash;
> - struct ovs_refcount refcount;
> -
> +/* State that flow translation can save, to restore when recirculation
> + * occurs. */
> +struct recirc_state {
> /* Initial table for post-recirculation processing. */
> uint8_t table_id;
>
> @@ -147,7 +140,25 @@ struct recirc_id_node {
> 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; /* Sequence of "struct ofpacts". */
> +};
> +
> +/* This maps a recirculation ID to saved state that flow translation can
> + * restore when recirculation occurs. */
> +struct recirc_id_node {
> + /* Index data. */
> + struct ovs_list exp_node OVS_GUARDED;
> + struct cmap_node id_node;
> + struct cmap_node metadata_node;
> + uint32_t id;
> + uint32_t hash;
> + struct ovs_refcount refcount;
> +
> + /* Saved state.
> + *
> + * This state should not be modified after inserting a node in the pool,
> + * hence the 'const' to emphasize that. */
> + const struct recirc_state state;
> };
>
> void recirc_init(void);
> @@ -156,14 +167,8 @@ void recirc_init(void);
> * updated to use this mechanism instead of internal rules. */
> uint32_t recirc_alloc_id(struct ofproto_dpif *);
>
> -uint32_t recirc_alloc_id_ctx(struct ofproto_dpif *, uint8_t table_id,
> - struct recirc_metadata *, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *);
> -uint32_t recirc_find_id(struct ofproto_dpif *, uint8_t table_id,
> - struct recirc_metadata *, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *);
> +uint32_t recirc_alloc_id_ctx(const struct recirc_state *);
> +uint32_t recirc_find_id(const struct recirc_state *);
> void recirc_free_id(uint32_t recirc_id);
> void recirc_free_ofproto(struct ofproto_dpif *, const char *ofproto_name);
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index cbcd2a3..4315e30 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3505,14 +3505,22 @@ compose_recirculate_action(struct xlate_ctx *ctx)
>
> ovs_assert(ctx->recirc_action_offset >= 0);
>
> + struct recirc_state state = {
> + .table_id = 0,
> + .ofproto = ctx->xbridge->ofproto,
> + .metadata = md,
> + .stack = &ctx->stack,
> + .action_set_len = ctx->recirc_action_offset,
> + .ofpacts_len = ctx->action_set.size,
> + .ofpacts = ctx->action_set.data,
> + };
> +
> /* Only allocate recirculation ID if we have a packet. */
> if (ctx->xin->packet) {
> /* Allocate a unique recirc id for the given metadata state in the
> * flow. The life-cycle of this recirc id is managed by associating it
> * with the udpif key ('ukey') created for each new datapath flow. */
> - id = recirc_alloc_id_ctx(ctx->xbridge->ofproto, 0, &md, &ctx->stack,
> - ctx->recirc_action_offset,
> - ctx->action_set.size, ctx->action_set.data);
> + id = recirc_alloc_id_ctx(&state);
> if (!id) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_ERR_RL(&rl, "Failed to allocate recirculation id");
> @@ -3522,12 +3530,12 @@ compose_recirculate_action(struct xlate_ctx *ctx)
> } else {
> /* Look up an existing recirc id for the given metadata state in the
> * flow. No new reference is taken, as the ID is RCU protected and is
> - * only required temporarily for verification. */
> - id = recirc_find_id(ctx->xbridge->ofproto, 0, &md, &ctx->stack,
> - ctx->recirc_action_offset,
> - ctx->action_set.size, ctx->action_set.data);
> - /* We let zero 'id' to be used in the RECIRC action below, which will
> - * fail all revalidations as zero is not a valid recirculation ID. */
> + * only required temporarily for verification.
> + *
> + * This might fail and return 0. We let zero 'id' to be used in the
> + * RECIRC action below, which will fail all revalidations as zero is
> + * not a valid recirculation ID. */
> + id = recirc_find_id(&state);
> }
>
> nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, id);
> @@ -4798,7 +4806,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> COVERAGE_INC(xlate_actions);
>
> if (xin->recirc) {
> - const struct recirc_id_node *recirc = xin->recirc;
> + const struct recirc_state *state = &xin->recirc->state;
>
> if (xin->ofpacts_len > 0 || ctx.rule) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> @@ -4811,10 +4819,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> }
>
> /* Set the bridge for post-recirculation processing if needed. */
> - if (ctx.xbridge->ofproto != recirc->ofproto) {
> + if (ctx.xbridge->ofproto != state->ofproto) {
> struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
> - const struct xbridge *new_bridge = xbridge_lookup(xcfg,
> - recirc->ofproto);
> + const struct xbridge *new_bridge
> + = xbridge_lookup(xcfg, state->ofproto);
>
> if (OVS_UNLIKELY(!new_bridge)) {
> /* Drop the packet if the bridge cannot be found. */
> @@ -4827,26 +4835,25 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>
> /* Set the post-recirculation table id. Note: A table lookup is done
> * only if there are no post-recirculation actions. */
> - ctx.table_id = recirc->table_id;
> + ctx.table_id = state->table_id;
>
> /* Restore pipeline metadata. May change flow's in_port and other
> * metadata to the values that existed when recirculation was
> * triggered. */
> - recirc_metadata_to_flow(&recirc->metadata, flow);
> + recirc_metadata_to_flow(&state->metadata, flow);
>
> /* Restore stack, if any. */
> - if (recirc->stack) {
> - ofpbuf_put(&ctx.stack, recirc->stack->data, recirc->stack->size);
> + if (state->stack) {
> + ofpbuf_put(&ctx.stack, state->stack->data, state->stack->size);
> }
>
> /* Restore action set, if any. */
> - if (recirc->action_set_len) {
> + if (state->action_set_len) {
> const struct ofpact *a;
>
> - ofpbuf_put(&ctx.action_set, recirc->ofpacts,
> - recirc->action_set_len);
> + ofpbuf_put(&ctx.action_set, state->ofpacts, state->action_set_len);
>
> - OFPACT_FOR_EACH(a, recirc->ofpacts, recirc->action_set_len) {
> + OFPACT_FOR_EACH(a, state->ofpacts, state->action_set_len) {
> if (a->type == OFPACT_GROUP) {
> ctx.action_set_has_group = true;
> break;
> @@ -4856,10 +4863,10 @@ 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 (recirc->ofpacts_len > recirc->action_set_len) {
> - xin->ofpacts_len = recirc->ofpacts_len - recirc->action_set_len;
> - xin->ofpacts = recirc->ofpacts +
> - recirc->action_set_len / sizeof *recirc->ofpacts;
> + 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;
> }
> } else if (OVS_UNLIKELY(flow->recirc_id)) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list