On 5 January 2016 at 16:24, Ben Pfaff <b...@ovn.org> wrote:
> revalidate_sweep__() has two cases where it calls ukey_delete() to
> remove a ukey from the umap via cmap_remove(). The first case is a direct
> call to ukey_delete(), when !flow_exists. The second case is an indirect
> call via push_ukey_ops(), when result != UKEY_KEEP. If both of these
> conditions are simultaneously true, however, the code would call
> ukey_delete() twice, causing an assertion failure in the second call. This
> commit fixes the problem by eliminating one of the calls.
>
> Reported-by: Keith Holleman <keith.holle...@gmail.com>
> Reported-at:
> http://openvswitch.org/pipermail/discuss/2015-December/019772.html
> CC: Joe Stringer <j...@ovn.org>
> VMware-BZ: #1579057
> Signed-off-by: Ben Pfaff <b...@ovn.org>
>
> ---
> ofproto/ofproto-dpif-upcall.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index cd8a9f0..ca25701 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -2274,7 +2274,7 @@ revalidator_sweep__(struct revalidator *revalidator,
> bool purge)
> n_ops = 0;
> }
>
> - if (!flow_exists) {
> + if (result == UKEY_KEEP && !flow_exists) {
> ovs_mutex_lock(&umap->mutex);
> ukey_delete(umap, ukey);
> ovs_mutex_unlock(&umap->mutex);
Thanks for finding this.
In the common case, I would expect the flow to be deleted during the
revalidator "dump" (mark) phase. With the current code, and with this
approach, we may end up attempting to delete the same flows multiple
times. I suggest the below instead:
@@ -2262,7 +2264,7 @@ revalidator_sweep__(struct revalidator
*revalidator, bool purge)
result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
reval_seq, &recircs);
}
- if (result != UKEY_KEEP) {
+ if (flow_exists && result != UKEY_KEEP) {
/* Takes ownership of 'recircs'. */
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
&odp_actions);
I notice a couple of other minor issues as well while looking at this code:
- We may currently revalidate ukeys in revalidator_sweep() whenever
the dump_seq/reval_seq mismatch, even if the ukey's corresponding flow
is already deleted. This is unnecessary: If we have previously deleted
the flow, we can just delete the ukey.
- If there is a seq mismatch in dump_seq or reval_seq, and
revalidate_ukey() returns UKEY_MODIFY, the flow will be modified and
the ukey deleted. I believe that this will not clear the datapath
stats, so next time the flow is dumped, all previous stats for the
flow will be double-attributed. This seems fairly unlikely as it
should only happen if the seqs change in between dump phase and
revalidation phase, or if the flow is not dumped by the kernel (as
well as only if revalidation requires modification of the flow - ie
actions changed but flows did not).
I will send patches for these latter issues.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
