[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU
From: |
Emilio G. Cota |
Subject: |
Re: [Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU |
Date: |
Fri, 31 Aug 2018 16:29:19 -0400 |
User-agent: |
Mutt/1.9.4 (2018-02-28) |
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
- [Qemu-devel] [PATCH v2 01/11] rcu_queue: use atomic_set in QLIST_REMOVE_RCU, (continued)
- [Qemu-devel] [PATCH v2 01/11] rcu_queue: use atomic_set in QLIST_REMOVE_RCU, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 10/11] spapr: do not use CPU_FOREACH_REVERSE, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 03/11] rcu_queue: add RCU QSIMPLEQ, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 11/11] qom: convert the CPU list to RCU, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 06/11] test-rcu-list: access counters with atomics, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 04/11] rcu_queue: add RCU QTAILQ, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 09/11] tests: add test-rcu-tailq, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 08/11] tests: add test-list-simpleq, Emilio G. Cota, 2018/08/19
- [Qemu-devel] [PATCH v2 05/11] test-rcu-list: access goflag with atomics, Emilio G. Cota, 2018/08/19
- Re: [Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU, Paolo Bonzini, 2018/08/20
- Re: [Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU,
Emilio G. Cota <=