[ovs-dev] [PATCH] revalidator: Replace ukey->mark with reval_seq.

[Joe Stringer]

Rather than setting and resetting the 'mark' field in the ukey, this
patch introduces a seq to track whether a flow has been seen during the
most recent dump. This tidies the code and simplifies the logic for
detecting when flows are duplicated from the datapath.

Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
 ofproto/ofproto-dpif-upcall.c |   34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 522aa8c..6adc729 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -167,8 +167,7 @@ struct udpif_key {
     struct ovs_mutex mutex;                   /* Guards the following. */
     struct dpif_flow_stats stats OVS_GUARDED; /* Last known stats.*/
     long long int created OVS_GUARDED;        /* Estimate of creation time. */
-    bool mark OVS_GUARDED;                    /* For mark and sweep garbage
-                                                 collection. */
+    uint64_t dump_seq OVS_GUARDED;            /* Tracks udpif->dump_seq. */
     bool flow_exists OVS_GUARDED;             /* Ensures flows are only deleted
                                                  once. */
 
@@ -1131,7 +1130,7 @@ ukey_create(const struct nlattr *key, size_t key_len, long long int used)
     ukey->key_len = key_len;
 
     ovs_mutex_lock(&ukey->mutex);
-    ukey->mark = false;
+    ukey->dump_seq = 0;
     ukey->flow_exists = true;
     ukey->created = used ? used : time_msec();
     memset(&ukey->stats, 0, sizeof ukey->stats);
@@ -1441,6 +1440,7 @@ revalidate(struct revalidator *revalidator)
     const struct nlattr *key, *mask, *actions;
     size_t key_len, mask_len, actions_len;
     const struct dpif_flow_stats *stats;
+    uint64_t dump_seq;
     long long int now;
     unsigned int flow_limit;
     size_t n_ops;
@@ -1448,13 +1448,14 @@ revalidate(struct revalidator *revalidator)
 
     n_ops = 0;
     now = time_msec();
+    dump_seq = seq_read(udpif->dump_seq);
     atomic_read(&udpif->flow_limit, &flow_limit);
 
     dpif_flow_dump_state_init(udpif->dpif, &state);
     while (dpif_flow_dump_next(&udpif->dump, state, &key, &key_len, &mask,
                                &mask_len, &actions, &actions_len, &stats)) {
         struct udpif_key *ukey;
-        bool mark, may_destroy;
+        bool keep, may_destroy;
         long long int used, max_idle;
         uint32_t hash;
         size_t n_flows;
@@ -1466,7 +1467,7 @@ revalidate(struct revalidator *revalidator)
         if (!used && ukey) {
             ovs_mutex_lock(&ukey->mutex);
 
-            if (ukey->mark || !ukey->flow_exists) {
+            if (ukey->dump_seq == dump_seq) {
                 /* The flow has already been dumped. This can occasionally
                  * occur if the datapath is changed in the middle of a flow
                  * dump. Rather than perform the same work twice, skip the
@@ -1480,6 +1481,7 @@ revalidate(struct revalidator *revalidator)
             ovs_mutex_unlock(&ukey->mutex);
         }
 
+        /* Delete flows more aggressively if over the limit. */
         n_flows = udpif_get_n_flows(udpif);
         max_idle = ofproto_max_idle;
         if (n_flows > flow_limit) {
@@ -1487,7 +1489,7 @@ revalidate(struct revalidator *revalidator)
         }
 
         if ((used && used < now - max_idle) || n_flows > flow_limit * 2) {
-            mark = false;
+            keep = false;
         } else {
             if (!ukey) {
                 ukey = ukey_create(key, key_len, used);
@@ -1500,17 +1502,18 @@ revalidate(struct revalidator *revalidator)
                 }
             }
 
-            mark = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
+            keep = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
                                    actions_len, stats);
         }
 
         if (ukey) {
             ovs_mutex_lock(&ukey->mutex);
-            ukey->mark = ukey->flow_exists = mark;
+            ukey->dump_seq = dump_seq;
+            ukey->flow_exists = keep;
             ovs_mutex_unlock(&ukey->mutex);
         }
 
-        if (!mark) {
+        if (!keep) {
             dump_op_init(&ops[n_ops++], key, key_len, ukey);
         }
 
@@ -1548,22 +1551,21 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
     struct dump_op ops[REVALIDATE_MAX_BATCH];
     struct udpif_key *ukey, *next;
     size_t n_ops;
+    uint64_t dump_seq;
 
     n_ops = 0;
+    dump_seq = seq_read(revalidator->udpif->dump_seq);
 
     /* During garbage collection, this revalidator completely owns its ukeys
      * map, and therefore doesn't need to do any locking. */
     HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, revalidator->ukeys) {
-        if (!purge && ukey->mark) {
-            ukey->mark = false;
-        } else if (!ukey->flow_exists) {
+        if (!ukey->flow_exists) {
             ukey_delete(revalidator, ukey);
-        } else {
+        } else if (purge || ukey->dump_seq != dump_seq) {
             struct dump_op *op = &ops[n_ops++];
 
-            /* If we have previously seen a flow in the datapath, but didn't
-             * see it during the most recent dump, delete it. This allows us
-             * to clean up the ukey and keep the statistics consistent. */
+            /* If we have previously seen a flow in the datapath, but it
+             * hasn't been revalidated with the current seq,  delete it. */
             dump_op_init(op, ukey->key, ukey->key_len, ukey);
             if (n_ops == REVALIDATE_MAX_BATCH) {
                 push_dump_ops(revalidator, ops, n_ops);
-- 
1.7.10.4