[ovs-dev] [RFC 1/3] revalidator: Don't delete missed dp flows immediately.

[Joe Stringer]

There is no guarantee that performing a flow_dump will return a complete
version of the datapath's flow table. Occasionally, a flow may be
completely missed even if it is present in the datapath (particularly if
the datapath decides to rehash its flow tables). Previously we would
delete such "missed flows" immediately. This patch gives those flows a
chance to be dumped, which should improve jitter/ latency for flows in
this corner case.

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

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 62ea48a..df929d6 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -185,7 +185,8 @@ 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. */
-    uint64_t dump_seq OVS_GUARDED;            /* Tracks udpif->dump_seq. */
+    int last_seen OVS_GUARDED;                /* For garbage-collecting flows
+                                                 not seen for a while. */
     bool flow_exists OVS_GUARDED;             /* Ensures flows are only deleted
                                                  once. */
 
@@ -195,6 +196,10 @@ struct udpif_key {
     struct odputil_keybuf key_buf;            /* Memory for 'key'. */
 };
 
+/* Maximum number of dumps that the datapath can fail to dump a flow before
+ * revalidators clean up the ukey and delete the flow. */
+#define MAX_UKEY_AGE 3
+
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 static struct list all_udpifs = LIST_INITIALIZER(&all_udpifs);
 
@@ -1041,7 +1046,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->dump_seq = 0;
+    ukey->last_seen = -1;
     ukey->flow_exists = true;
     ukey->created = used ? used : time_msec();
     memset(&ukey->stats, 0, sizeof ukey->stats);
@@ -1355,10 +1360,8 @@ revalidate(struct revalidator *revalidator)
 {
     struct udpif *udpif = revalidator->udpif;
     struct dpif_flow_dump_thread *dump_thread;
-    uint64_t dump_seq;
     unsigned int flow_limit;
 
-    dump_seq = seq_read(udpif->dump_seq);
     atomic_read(&udpif->flow_limit, &flow_limit);
     dump_thread = dpif_flow_dump_thread_create(udpif->dump);
     for (;;) {
@@ -1410,7 +1413,7 @@ revalidate(struct revalidator *revalidator)
                 continue;
             }
 
-            already_dumped = ukey->dump_seq == dump_seq;
+            already_dumped = ukey->last_seen == 0;
             if (already_dumped) {
                 /* The flow has already been dumped and handled by another
                  * revalidator during this flow dump operation. Skip it. */
@@ -1427,10 +1430,11 @@ revalidate(struct revalidator *revalidator)
             } else {
                 keep = revalidate_ukey(udpif, ukey, f);
             }
-            ukey->dump_seq = dump_seq;
             ukey->flow_exists = keep;
 
-            if (!keep) {
+            if (keep) {
+                ukey->last_seen = 0;
+            } else {
                 dump_op_init(&ops[n_ops++], f->key, f->key_len, ukey);
             }
             ovs_mutex_unlock(&ukey->mutex);
@@ -1450,21 +1454,26 @@ 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;
+    bool need_revalidate;
 
     n_ops = 0;
-    dump_seq = seq_read(revalidator->udpif->dump_seq);
+    need_revalidate = revalidator->udpif->need_revalidate;
 
     /* 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) {
+        bool missed_reval;
+
+        missed_reval = ukey->last_seen && need_revalidate;
+        ukey->last_seen++;
         if (!ukey->flow_exists) {
             ukey_delete(revalidator, ukey);
-        } else if (purge || ukey->dump_seq != dump_seq) {
+        } else if (purge || missed_reval || ukey->last_seen == MAX_UKEY_AGE) {
             struct dump_op *op = &ops[n_ops++];
 
-            /* If we have previously seen a flow in the datapath, but it
-             * hasn't been seen in the current dump, delete it. */
+            /* 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. */
             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