While correct, I would find this easier to follow (as an incremental on top of
this patch):
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index d611131..390acbf 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2240,9 +2240,8 @@ 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;
+ bool flow_exists;
struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
- enum reval_result result;
/* 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 +2249,40 @@ 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 || !flow_exists) {
- 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) {
+ enum reval_result result;
+ bool 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 (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) {
Your call. Either way:
Acked-by: Jarno Rajahalme <[email protected]>
> On Jan 7, 2016, at 11:47 AM, Joe Stringer <[email protected]> wrote:
>
> Flows which are deleted during the revalidator "dump" phase have their
> corresponding dump seq updated, which leads to such ukeys being handled
> by the "UKEY_KEEP" logic in revalidator_sweep__() here:
>
> } else if (!seq_mismatch) {
> result = UKEY_KEEP;
> }
>
> In practice, this has allowed the flow to be deleted in revalidate(),
> then skip the flow deletion step in revalidator_sweep__(), and finally
> delete the ukey itself at the end of revalidator_sweep__().
>
> However, this is also misleading because the ukeys which are being
> deleted are considered under the "UKEY_KEEP" logic. 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 <[email protected]>
> ---
> ofproto/ofproto-dpif-upcall.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 69a04c9a50ca..d61113176842 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2253,7 +2253,7 @@ revalidator_sweep__(struct revalidator *revalidator,
> bool purge)
> seq_mismatch = (ukey->dump_seq != dump_seq
> && ukey->reval_seq != reval_seq);
>
> - if (purge) {
> + if (purge || !flow_exists) {
> result = UKEY_DELETE;
> } else if (!seq_mismatch) {
> result = UKEY_KEEP;
> --
> 2.1.4
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
