Skip to content

Commit

Permalink
ofproto-dpif-rid: Use separate pointers for actions and action set.
Browse files Browse the repository at this point in the history
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 <[email protected]>
Acked-by: Jarno Rajahalme <[email protected]>
  • Loading branch information
blp committed Jan 21, 2016
1 parent 5c1b231 commit 417509f
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 16 deletions.
13 changes: 11 additions & 2 deletions ofproto/ofproto-dpif-rid.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -216,13 +221,17 @@ recirc_state_clone(struct recirc_state *new, const struct recirc_state *old,
new->ofpacts = (new->ofpacts_len
? xmemdup(new->ofpacts, new->ofpacts_len)
: NULL);
new->action_set = (new->action_set_len
? xmemdup(new->action_set, new->action_set_len)
: NULL);
}

static void
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.
Expand Down
8 changes: 4 additions & 4 deletions ofproto/ofproto-dpif-rid.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 11 additions & 10 deletions ofproto/ofproto-dpif-xlate.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand Down

0 comments on commit 417509f

Please sign in to comment.