ofproto-dpif-upcall: Simplify revalidator_sweep__().

Broadly, there are two cases that are handled during revalidator_sweep__:
- Ukeys which had their corresponding datapath flows deleted during the
  most recent dump phase need to be deleted.
- If a flow for a ukey still exists in the datapath, the flow may need
  to be removed or updated. This depends on a variety of factors such as
  whether the datapath is being flushed, whether individual flows were
  recently dumped, and whether those flows are valid for the current
  revalidation generation.

Previously, the logic was written such that the first of these cases
would be handled under the "UKEY_KEEP" case to ensure that
revalidator_sweep__() will not attempt to delete flows that already
exist. In this case, ukey->flow_exists would be false, which would
trigger ukey cleanup.

While correct, this is misleading and difficult to follow. Since commit
83b03fe05e7a ("ofproto-dpif-upcall: Avoid double-delete of ukeys."),
this logic is no longer required to prevent double-deletion of such
flows, so we can now make this codepath more straightforward.

Signed-off-by: Joe Stringer <joe@ovn.org>
Co-authored-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Jarno Rajahalme <jarno@ovn.org>

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 69a04c9..f0c9294 100644 (file)
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2240,9 +2240,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
         size_t n_ops = 0;
 
         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;
+            bool flow_exists;
 
             /* Handler threads could be holding a ukey lock while it installs a
              * new flow, so don't hang around waiting for access to it. */
@@ -2250,37 +2248,41 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
                 continue;
             }
             flow_exists = ukey->flow_exists;
-            seq_mismatch = (ukey->dump_seq != dump_seq
-                            && ukey->reval_seq != reval_seq);
-
-            if (purge) {
-                result = UKEY_DELETE;
-            } else if (!seq_mismatch) {
-                result = UKEY_KEEP;
-            } else {
-                struct dpif_flow_stats stats;
-                COVERAGE_INC(revalidate_missed_dp_flow);
-                memset(&stats, 0, sizeof stats);
-                result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
-                                         reval_seq, &recircs);
-            }
-            if (flow_exists && result != UKEY_KEEP) {
-                /* Takes ownership of 'recircs'. */
-                reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
-                              &odp_actions);
+            if (flow_exists) {
+                struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
+                bool seq_mismatch = (ukey->dump_seq != dump_seq
+                                     && ukey->reval_seq != reval_seq);
+                enum reval_result result;
+
+                if (purge) {
+                    result = UKEY_DELETE;
+                } else if (!seq_mismatch) {
+                    result = UKEY_KEEP;
+                } else {
+                    struct dpif_flow_stats stats;
+                    COVERAGE_INC(revalidate_missed_dp_flow);
+                    memset(&stats, 0, sizeof stats);
+                    result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
+                                             reval_seq, &recircs);
+                }
+                if (result != UKEY_KEEP) {
+                    /* Clears 'recircs' if filled by revalidate_ukey(). */
+                    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);
-                n_ops = 0;
-            }
-
             if (!flow_exists) {
                 ovs_mutex_lock(&umap->mutex);
                 ukey_delete(umap, ukey);
                 ovs_mutex_unlock(&umap->mutex);
             }
+
+            if (n_ops == REVALIDATE_MAX_BATCH) {
+                push_ukey_ops(udpif, umap, ops, n_ops);
+                n_ops = 0;
+            }
         }
 
         if (n_ops) {