From cebfec69420bc838df002c51b560488c81bf33ee Mon Sep 17 00:00:00 2001 From: Joe Stringer Date: Thu, 7 Jan 2016 11:47:47 -0800 Subject: [PATCH] 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 Co-authored-by: Jarno Rajahalme Acked-by: Jarno Rajahalme --- ofproto/ofproto-dpif-upcall.c | 54 ++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 69a04c9a5..f0c9294eb 100644 --- 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) { -- 2.20.1