Re: [PATCH 4/4] async: use explicit memory barriers and relaxed accesses

From: Stefan Hajnoczi <stefanha@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ying Fang <fangying1@huawei.com>, qemu-devel@nongnu.org
Subject: Re: [PATCH 4/4] async: use explicit memory barriers and relaxed accesses
Date: Tue, 7 Apr 2020 10:09:54 +0100	[thread overview]
Message-ID: <20200407090954.GC247777@stefanha-x1.localdomain> (raw)
In-Reply-To: <20200406191320.13371-5-pbonzini@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

On Mon, Apr 06, 2020 at 03:13:20PM -0400, Paolo Bonzini wrote:
> When using C11 atomics, non-seqcst reads and writes do not participate
> in the total order of seqcst operations.  In util/async.c and util/aio-posix.c,
> in particular, the pattern that we use
> 
>           write ctx->notify_me                 write bh->scheduled
>           read bh->scheduled                   read ctx->notify_me
>           if !bh->scheduled, sleep             if ctx->notify_me, notify
> 
> needs to use seqcst operations for both the write and the read.  In
> general this is something that we do not want, because there can be
> many sources that are polled in addition to bottom halves.  The
> alternative is to place a seqcst memory barrier between the write
> and the read.  This also comes with a disadvantage, in that the
> memory barrier is implicit on strongly-ordered architectures and
> it wastes a few dozen clock cycles.
> 
> Fortunately, ctx->notify_me is never written concurrently by two
> threads, so we can instead relax the writes to ctx->notify_me.
> [This part of the commit message is still to be written more
> in detail and is what I am not sure about.]
> 
> Analyzed-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/aio-posix.c |  9 ++++++++-
>  util/aio-win32.c |  8 +++++++-
>  util/async.c     | 12 ++++++++++--
>  3 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index cd6cf0a4a9..37afec726f 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -569,7 +569,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
>       * so disable the optimization now.
>       */
>      if (blocking) {
> -        atomic_add(&ctx->notify_me, 2);
> +        atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);

Non-atomic "atomic" code looks suspicious and warrants a comment
mentioning that this is only executed from one thread.  This applies to
the other instances in this patch too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]