[ovs-dev] [PATCH v2 2/8] ofproto: Enable in-place modification for recirc actions.

Jarno Rajahalme jrajahalme at nicira.com
Sat Nov 7 00:10:49 UTC 2015


When modifying an existing datapath flow with recirculation actions,
the references to old (if any) recirculation actions need to be freed,
and references to new recirculation actions need to be stored.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
Acked-by: Joe Stringer <joestringer at nicira.com>
---
 ofproto/ofproto-dpif-rid.h    | 63 ++++++++++++++++++++++++++++
 ofproto/ofproto-dpif-upcall.c | 96 +++++++++++++++++++++++++++----------------
 ofproto/ofproto-dpif-xlate.c  | 36 +++++-----------
 ofproto/ofproto-dpif-xlate.h  | 62 +---------------------------
 4 files changed, 136 insertions(+), 121 deletions(-)

diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
index 6ac5fef..135810d 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -196,4 +196,67 @@ void recirc_id_node_unref(const struct recirc_id_node *);
 
 void recirc_run(void);
 
+/* Recirculation IDs on which references are held. */
+struct recirc_refs {
+    unsigned n_recircs;
+    union {
+        uint32_t recirc[2];   /* When n_recircs == 1 or 2 */
+        uint32_t *recircs;    /* When 'n_recircs' > 2 */
+    };
+};
+
+#define RECIRC_REFS_EMPTY_INITIALIZER ((struct recirc_refs) \
+                                       { 0, { { 0, 0 } } })
+/* Helpers to abstract the recirculation union away. */
+static inline void
+recirc_refs_init(struct recirc_refs *rr)
+{
+    *rr = RECIRC_REFS_EMPTY_INITIALIZER;
+}
+
+static inline void
+recirc_refs_add(struct recirc_refs *rr, uint32_t id)
+{
+    if (OVS_LIKELY(rr->n_recircs < ARRAY_SIZE(rr->recirc))) {
+        rr->recirc[rr->n_recircs++] = id;
+    } else {
+        if (rr->n_recircs == ARRAY_SIZE(rr->recirc)) {
+            uint32_t *recircs = xmalloc(sizeof rr->recirc + sizeof id);
+
+            memcpy(recircs, rr->recirc, sizeof rr->recirc);
+            rr->recircs = recircs;
+        } else {
+            rr->recircs = xrealloc(rr->recircs,
+                                   (rr->n_recircs + 1) * sizeof id);
+        }
+        rr->recircs[rr->n_recircs++] = id;
+    }
+}
+
+static inline void
+recirc_refs_swap(struct recirc_refs *a, struct recirc_refs *b)
+{
+    struct recirc_refs tmp;
+
+    tmp = *a;
+    *a = *b;
+    *b = tmp;
+}
+
+static inline void
+recirc_refs_unref(struct recirc_refs *rr)
+{
+    if (OVS_LIKELY(rr->n_recircs <= ARRAY_SIZE(rr->recirc))) {
+        for (int i = 0; i < rr->n_recircs; i++) {
+            recirc_free_id(rr->recirc[i]);
+        }
+    } else {
+        for (int i = 0; i < rr->n_recircs; i++) {
+            recirc_free_id(rr->recircs[i]);
+        }
+        free(rr->recircs);
+    }
+    rr->n_recircs = 0;
+}
+
 #endif
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 6aa1aad..2734696 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -261,9 +261,8 @@ struct udpif_key {
         struct nlattr nla;
     } keybuf, maskbuf;
 
-    /* Recirculation IDs with references held by the ukey. */
-    unsigned n_recircs;
-    uint32_t recircs[];   /* 'n_recircs' id's for which references are held. */
+    uint32_t key_recirc_id;   /* Non-zero if reference is held by the ukey. */
+    struct recirc_refs recircs;  /* Action recirc IDs with references held. */
 };
 
 /* Datapath operation with optional ukey attached. */
@@ -1450,12 +1449,10 @@ ukey_create__(const struct nlattr *key, size_t key_len,
               bool ufid_present, const ovs_u128 *ufid,
               const unsigned pmd_id, const struct ofpbuf *actions,
               uint64_t dump_seq, uint64_t reval_seq, long long int used,
-              const struct recirc_id_node *key_recirc, struct xlate_out *xout)
+              uint32_t key_recirc_id, struct xlate_out *xout)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
-    unsigned n_recircs = (key_recirc ? 1 : 0) + (xout ? xout->n_recircs : 0);
-    struct udpif_key *ukey = xmalloc(sizeof *ukey +
-                                     n_recircs * sizeof *ukey->recircs);
+    struct udpif_key *ukey = xmalloc(sizeof *ukey);
 
     memcpy(&ukey->keybuf, key, key_len);
     ukey->key = &ukey->keybuf.nla;
@@ -1480,17 +1477,13 @@ ukey_create__(const struct nlattr *key, size_t key_len,
     ukey->stats.used = used;
     ukey->xcache = NULL;
 
-    ukey->n_recircs = n_recircs;
-    if (key_recirc) {
-        ukey->recircs[0] = key_recirc->id;
+    ukey->key_recirc_id = key_recirc_id;
+    recirc_refs_init(&ukey->recircs);
+    if (xout) {
+        /* Take ownership of the action recirc id references. */
+        recirc_refs_swap(&ukey->recircs, &xout->recircs);
     }
-    if (xout && xout->n_recircs) {
-        const uint32_t *act_recircs = xlate_out_get_recircs(xout);
 
-        memcpy(ukey->recircs + (key_recirc ? 1 : 0), act_recircs,
-               xout->n_recircs * sizeof *ukey->recircs);
-        xlate_out_take_recircs(xout);
-    }
     return ukey;
 }
 
@@ -1529,7 +1522,7 @@ ukey_create_from_upcall(struct upcall *upcall, struct flow_wildcards *wc)
                          true, upcall->ufid, upcall->pmd_id,
                          &upcall->put_actions, upcall->dump_seq,
                          upcall->reval_seq, 0,
-                         upcall->have_recirc_ref ? upcall->recirc : NULL,
+                         upcall->have_recirc_ref ? upcall->recirc->id : 0,
                          &upcall->xout);
 }
 
@@ -1582,7 +1575,7 @@ ukey_create_from_dpif_flow(const struct udpif *udpif,
     *ukey = ukey_create__(flow->key, flow->key_len,
                           flow->mask, flow->mask_len, flow->ufid_present,
                           &flow->ufid, flow->pmd_id, &actions, dump_seq,
-                          reval_seq, flow->stats.used, NULL, NULL);
+                          reval_seq, flow->stats.used, 0, NULL);
 
     return 0;
 }
@@ -1725,9 +1718,10 @@ ukey_delete__(struct udpif_key *ukey)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     if (ukey) {
-        for (int i = 0; i < ukey->n_recircs; i++) {
-            recirc_free_id(ukey->recircs[i]);
+        if (ukey->key_recirc_id) {
+            recirc_free_id(ukey->key_recirc_id);
         }
+        recirc_refs_unref(&ukey->recircs);
         xlate_cache_delete(ukey->xcache);
         ofpbuf_delete(ovsrcu_get(struct ofpbuf *, &ukey->actions));
         ovs_mutex_destroy(&ukey->mutex);
@@ -1784,11 +1778,21 @@ should_revalidate(const struct udpif *udpif, uint64_t packets,
  *      UKEY_KEEP   The ukey is fine as is.
  *      UKEY_MODIFY The ukey's actions should be changed but is otherwise
  *                  fine.  Callers should change the actions to those found
- *                  in the caller supplied 'odp_actions' buffer. */
+ *                  in the caller supplied 'odp_actions' buffer.  The
+ *                  recirculation references can be found in 'recircs' and
+ *                  must be handled by the caller.
+ *
+ * If the result is UKEY_MODIFY, then references to all recirc_ids used by the
+ * new flow will be held within 'recircs' (which may be none).
+ *
+ * The caller is responsible for both initializing 'recircs' prior this call,
+ * and ensuring any references are eventually freed.
+ */
 static enum reval_result
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
                 const struct dpif_flow_stats *stats,
-                struct ofpbuf *odp_actions, uint64_t reval_seq)
+                struct ofpbuf *odp_actions, uint64_t reval_seq,
+                struct recirc_refs *recircs)
     OVS_REQUIRES(ukey->mutex)
 {
     struct xlate_out xout, *xoutp;
@@ -1902,6 +1906,8 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
         /* The datapath mask was OK, but the actions seem to have changed.
          * Let's modify it in place. */
         result = UKEY_MODIFY;
+        /* Transfer recirc action ID references to the caller. */
+        recirc_refs_swap(recircs, &xoutp->recircs);
         goto exit;
     }
 
@@ -2073,6 +2079,25 @@ log_unexpected_flow(const struct dpif_flow *flow, int error)
     VLOG_WARN_RL(&rl, "%s", ds_cstr(&ds));
 }
 
+static void reval_op_init(struct ukey_op *op, enum reval_result result,
+                          struct udpif *udpif, struct udpif_key *ukey,
+                          struct recirc_refs *recircs,
+                          struct ofpbuf *odp_actions)
+{
+    if (result == UKEY_DELETE) {
+        delete_op_init(udpif, op, ukey);
+    } else if (result == UKEY_MODIFY) {
+        /* Store the new recircs. */
+        recirc_refs_swap(&ukey->recircs, recircs);
+        /* Release old recircs. */
+        recirc_refs_unref(recircs);
+        /* ukey->key_recirc_id remains, as the key is the same as before. */
+
+        ukey_set_actions(ukey, odp_actions);
+        modify_op_init(op, ukey);
+    }
+}
+
 static void
 revalidate(struct revalidator *revalidator)
 {
@@ -2126,6 +2151,7 @@ revalidate(struct revalidator *revalidator)
 
         for (f = flows; f < &flows[n_dumped]; f++) {
             long long int used = f->stats.used;
+            struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
             enum reval_result result;
             struct udpif_key *ukey;
             bool already_dumped;
@@ -2165,16 +2191,15 @@ revalidate(struct revalidator *revalidator)
                 result = UKEY_DELETE;
             } else {
                 result = revalidate_ukey(udpif, ukey, &f->stats, &odp_actions,
-                                         reval_seq);
+                                         reval_seq, &recircs);
             }
             ukey->dump_seq = dump_seq;
             ukey->flow_exists = result != UKEY_DELETE;
 
-            if (result == UKEY_DELETE) {
-                delete_op_init(udpif, &ops[n_ops++], ukey);
-            } else if (result == UKEY_MODIFY) {
-                ukey_set_actions(ukey, &odp_actions);
-                modify_op_init(&ops[n_ops++], ukey);
+            if (result != UKEY_KEEP) {
+                /* Takes ownership of 'recircs'. */
+                reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
+                              &odp_actions);
             }
             ovs_mutex_unlock(&ukey->mutex);
         }
@@ -2223,6 +2248,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
 
         CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
             bool flow_exists, seq_mismatch;
+            struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
             enum reval_result result;
 
             /* Handler threads could be holding a ukey lock while it installs a
@@ -2243,16 +2269,14 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
                 COVERAGE_INC(revalidate_missed_dp_flow);
                 memset(&stats, 0, sizeof stats);
                 result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
-                                         reval_seq);
+                                         reval_seq, &recircs);
             }
-            ovs_mutex_unlock(&ukey->mutex);
-
-            if (result == UKEY_DELETE) {
-                delete_op_init(udpif, &ops[n_ops++], ukey);
-            } else if (result == UKEY_MODIFY) {
-                ukey_set_actions(ukey, &odp_actions);
-                modify_op_init(&ops[n_ops++], ukey);
+            if (result != UKEY_KEEP) {
+                /* Takes ownership of 'recircs'. */
+                reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
+                              &odp_actions);
             }
+            ovs_mutex_unlock(&ukey->mutex);
 
             if (n_ops == REVALIDATE_MAX_BATCH) {
                 push_ukey_ops(udpif, umap, ops, n_ops);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 192d0fa..6baea7f 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3588,30 +3588,16 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table)
         .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(&state);
-        if (!id) {
-            XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id");
-            ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
-            return;
-        }
-        xlate_out_add_recirc(ctx->xout, id);
-    } 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.
-         * If flow tables have changed sufficiently this can fail and we will
-         * delete the old datapath flow. */
-        id = recirc_find_id(&state);
-        if (!id) {
-            ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
-            return;
-        }
+    /* 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(&state);
+    if (!id) {
+        XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id");
+        ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
+        return;
     }
+    recirc_refs_add(&ctx->xout->recircs, id);
 
     nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, id);
 
@@ -4708,7 +4694,7 @@ void
 xlate_out_uninit(struct xlate_out *xout)
 {
     if (xout) {
-        xlate_out_free_recircs(xout);
+        recirc_refs_unref(&xout->recircs);
     }
 }
 
@@ -4922,7 +4908,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     *xout = (struct xlate_out) {
         .slow = 0,
         .fail_open = false,
-        .n_recircs = 0,
+        .recircs = RECIRC_REFS_EMPTY_INITIALIZER,
     };
 
     struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index ca63b1f..8aaae1a 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -41,68 +41,10 @@ struct xlate_out {
     enum slow_path_reason slow; /* 0 if fast path may be used. */
     bool fail_open;             /* Initial rule is fail open? */
 
-    /* Recirculation IDs on which references are held. */
-    unsigned n_recircs;
-    union {
-        uint32_t recirc[2];   /* When n_recircs == 1 or 2 */
-        uint32_t *recircs;    /* When 'n_recircs' > 2 */
-    };
+    struct recirc_refs recircs; /* Recirc action IDs on which references are
+                                 * held. */
 };
 
-/* Helpers to abstract the recirculation union away. */
-static inline void
-xlate_out_add_recirc(struct xlate_out *xout, uint32_t id)
-{
-    if (OVS_LIKELY(xout->n_recircs < ARRAY_SIZE(xout->recirc))) {
-        xout->recirc[xout->n_recircs++] = id;
-    } else {
-        if (xout->n_recircs == ARRAY_SIZE(xout->recirc)) {
-            uint32_t *recircs = xmalloc(sizeof xout->recirc + sizeof id);
-
-            memcpy(recircs, xout->recirc, sizeof xout->recirc);
-            xout->recircs = recircs;
-        } else {
-            xout->recircs = xrealloc(xout->recircs,
-                                     (xout->n_recircs + 1) * sizeof id);
-        }
-        xout->recircs[xout->n_recircs++] = id;
-    }
-}
-
-static inline const uint32_t *
-xlate_out_get_recircs(const struct xlate_out *xout)
-{
-    if (OVS_LIKELY(xout->n_recircs <= ARRAY_SIZE(xout->recirc))) {
-        return xout->recirc;
-    } else {
-        return xout->recircs;
-    }
-}
-
-static inline void
-xlate_out_take_recircs(struct xlate_out *xout)
-{
-    if (OVS_UNLIKELY(xout->n_recircs > ARRAY_SIZE(xout->recirc))) {
-        free(xout->recircs);
-    }
-    xout->n_recircs = 0;
-}
-
-static inline void
-xlate_out_free_recircs(struct xlate_out *xout)
-{
-    if (OVS_LIKELY(xout->n_recircs <= ARRAY_SIZE(xout->recirc))) {
-        for (int i = 0; i < xout->n_recircs; i++) {
-            recirc_free_id(xout->recirc[i]);
-        }
-    } else {
-        for (int i = 0; i < xout->n_recircs; i++) {
-            recirc_free_id(xout->recircs[i]);
-        }
-        free(xout->recircs);
-    }
-}
-
 struct xlate_in {
     struct ofproto_dpif *ofproto;
 
-- 
2.1.4




More information about the dev mailing list