> On Aug 10, 2015, at 6:46 PM, Ethan J. Jackson <[email protected]> wrote:
>
> From: Ethan Jackson <[email protected]>
>
> Since revalidator_sweep() doesn't hold the ukey mutex for each full
> loop iteration, it's theoretically possible that two threads may
> call ukey_delete() on the same ukey. If this happens, they both will
> attempt to remove the ukey from he cmap, causing the loser of the race
> to fail.
>
> Signed-off-by: Ethan J. Jackson <[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 0f2e186..fddb535 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2076,7 +2076,6 @@ revalidator_sweep__(struct revalidator *revalidator,
> bool purge)
> flow_exists = ukey->flow_exists;
> seq_mismatch = (ukey->dump_seq != dump_seq
> && ukey->reval_seq != reval_seq);
> - ovs_mutex_unlock(&ukey->mutex);
>
> if (flow_exists
> && (purge
There is a call to push_ukey_ops() between these blocks. It calls
push_ukey_ops__(), which will take a lock on the ukeys, including the last one
added and still locked, so this would result in double locking.
This could be resolved by moving the “ if (n_ops == REVALIDATE_MAX_BATCH)”
block after the (moved) ukey unlock, similarly to the remainder ops handling at
the end. However, I’m not sure if this would defeat the purpose of this patch.
Also, it seems that generally the umap lock is taken first and ukey locks
after, so there is a possibility of a deadlock if the locks are taken in the
reverse order, like in this patch.
It would be really great if we could employ clang thread safety analysis more
than we do in this file currently, but I’m not sure if that is possible.
Jarno
> @@ -2095,6 +2094,7 @@ revalidator_sweep__(struct revalidator *revalidator,
> bool purge)
> ukey_delete(umap, ukey);
> ovs_mutex_unlock(&umap->mutex);
> }
> + ovs_mutex_unlock(&ukey->mutex);
> }
>
> if (n_ops) {
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
