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 <b...@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@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
