Re: [Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU

["Emilio G. Cota" <cota@braap.org>]

On Mon, Aug 20, 2018 at 11:30:07 +0200, Paolo Bonzini wrote:
> On 19/08/2018 11:13, Emilio G. Cota wrote:
> > - Add some fixes for test-rcu-list. I wanted to be able to get no
> >   races with ThreadSanitizer, but it still warns about two races.
> >   I'm appending the report just in case, but I think tsan is getting
> >   confused.
> 
> I cannot understand the first.  The second might be fixed by this untested
> patch (the second hunk; the first is an optimization and clarification):
> 
> diff --git a/util/rcu.c b/util/rcu.c
> index 5676c22bd1..314b7db1a6 100644
> --- a/util/rcu.c
> +++ b/util/rcu.c
> @@ -192,7 +192,9 @@ static void enqueue(struct rcu_head *node)
>  
>      node->next = NULL;
>      old_tail = atomic_xchg(&tail, &node->next);
> -    atomic_mb_set(old_tail, node);
> +
> +    /* Pairs with smb_mb_acquire() in try_dequeue.  */
> +    atomic_store_release(old_tail, node);
>  }
>  
>  static struct rcu_head *try_dequeue(void)
> @@ -213,7 +215,10 @@ retry:
>       * wrong and we need to wait until its enqueuer finishes the update.
>       */
>      node = head;
> -    next = atomic_mb_read(&head->next);
> +    smp_mb_acquire();
> +
> +    /* atomic_read is enough because the pointer is never dereferenced.  */
> +    next = atomic_read(&head->next);
>      if (!next) {
>          return NULL;
>      }
> 
> 
> The idea being that enqueue() writes so to speak node->prev->next in
> the atomic_store_release when it enqueues node; try_dequeue() instead reads
> node->next, so there is no synchronizes-with edge.  However, I'm not that
> convinced that it's necessary...

This doesn't seem to help :-( I'd try to avoid standalone barriers
as much as possible, otherwise TSan gets confused (BTW this is why
seqlocks cannot be "understood" by TSan).

The cause of the warnings seem to be the calls to g_usleep (both in rcu.c
and in the test file), which lead TSan to believe that we might be
coordinating threads via sleep calls.

Substituting the sleep calls with cpu_relax gets rid of the warnings,
so I would say these warnings can be safely ignored.

Thanks,

		Emilio