[ovs-dev] [PATCH 35/41] ofproto-dpif-rid: Use separate pointers for actions and action set.

[Ben Pfaff]

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