Re: [PATCH 1/4] atomics: convert to reStructuredText

From: Eric Blake <eblake@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org
Cc: Ying Fang <fangying1@huawei.com>, stefanha@redhat.com
Subject: Re: [PATCH 1/4] atomics: convert to reStructuredText
Date: Mon, 6 Apr 2020 14:58:59 -0500	[thread overview]
Message-ID: <d0585240-030b-00c3-cece-bec1d2fa2870@redhat.com> (raw)
In-Reply-To: <20200406191320.13371-2-pbonzini@redhat.com>

On 4/6/20 2:13 PM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   docs/devel/atomics.rst | 447 +++++++++++++++++++++++++++++++++++++++++
>   docs/devel/atomics.txt | 403 -------------------------------------
>   docs/devel/index.rst   |   1 +
>   3 files changed, 448 insertions(+), 403 deletions(-)
>   create mode 100644 docs/devel/atomics.rst
>   delete mode 100644 docs/devel/atomics.txt
> 

Pre-existing grammar nits, that you may want to touch up while reformatting:

> +Compiler memory barrier
> +=======================
> +
> +``barrier()`` prevents the compiler from moving the memory accesses either
> +side of it to the other side.  The compiler barrier has no direct effect

s/either/on either/

> +``qemu/atomic.h`` provides the following set of atomic read-modify-write
> +operations::
> +
> +    void atomic_inc(ptr)
> +    void atomic_dec(ptr)
> +    void atomic_add(ptr, val)
> +    void atomic_sub(ptr, val)
> +    void atomic_and(ptr, val)
> +    void atomic_or(ptr, val)
> +
> +    typeof(*ptr) atomic_fetch_inc(ptr)
> +    typeof(*ptr) atomic_fetch_dec(ptr)
> +    typeof(*ptr) atomic_fetch_add(ptr, val)
> +    typeof(*ptr) atomic_fetch_sub(ptr, val)
> +    typeof(*ptr) atomic_fetch_and(ptr, val)
> +    typeof(*ptr) atomic_fetch_or(ptr, val)
> +    typeof(*ptr) atomic_fetch_xor(ptr, val)
> +    typeof(*ptr) atomic_fetch_inc_nonzero(ptr)
> +    typeof(*ptr) atomic_xchg(ptr, val)
> +    typeof(*ptr) atomic_cmpxchg(ptr, old, new)
> +
> +all of which return the old value of ``*ptr``.  These operations are
> +polymorphic; they operate on any type that is as wide as a pointer.

Is th is 'as wide as a pointer' or 'no wider than a pointer'? In other 
words, can 'val' be a narrower type?

> +However, and this is the important difference between
> +atomic_mb_read/atomic_mb_set and sequential consistency, it is important
> +for both threads to access the same volatile variable.  It is not the
> +case that everything visible to thread A when it writes volatile field f
> +becomes visible to thread B after it reads volatile field g. The store
> +and load have to "match" (i.e., be performed on the same volatile
> +field) to achieve the right semantics.
> +
> +
> +These operations operate on any type that is as wide as an int or smaller.

Is that all operations in this same section, or only the last set of two 
operations (atomic_mb_read/set)?  What is the appropriate code to use if 
a pointer is wider than int?

> +
> +You can see that the two possible definitions of ``atomic_mb_read()``
> +and ``atomic_mb_set()`` are the following:
> +
> +  1) | atomic_mb_read(p)   = atomic_read(p); smp_mb_acquire()
> +     | atomic_mb_set(p, v) = smp_mb_release(); atomic_set(p, v); smp_mb()
> +
> +  2) | atomic_mb_read(p)   = smp_mb() atomic_read(p); smp_mb_acquire()

Missing semicolon after smp_mb()

> +     | atomic_mb_set(p, v) = smp_mb_release(); atomic_set(p, v);
> +
> +Usually the former is used, because ``smp_mb()`` is expensive and a program
> +normally has more reads than writes.  Therefore it makes more sense to
> +make ``atomic_mb_set()`` the more expensive operation.
> +

> +Memory barrier pairing
> +----------------------
> +
> +A useful rule of thumb is that memory barriers should always, or almost
> +always, be paired with another barrier.  In the case of QEMU, however,
> +note that the other barrier may actually be in a driver that runs in
> +the guest!
> +
> +For the purposes of pairing, ``smp_read_barrier_depends()`` and ``smp_rmb()``
> +both count as read barriers.  A read barrier shall pair with a write
> +barrier or a full barrier; a write barrier shall pair with a read

'shall' is awkward (if this is not a formal RFC-style requirement), 
better for colloquial English is 'must' or 'should' (twice)

> +barrier or a full barrier.  A full barrier can pair with anything.

> +Comparison with Linux kernel memory barriers
> +============================================
> +
> +Here is a list of differences between Linux kernel atomic operations
> +and memory barriers, and the equivalents in QEMU:
> +
> +- atomic operations in Linux are always on a 32-bit int type and
> +  use a boxed atomic_t type; atomic operations in QEMU are polymorphic
> +  and use normal C types.
> +
> +- Originally, atomic_read and atomic_set in Linux gave no guarantee
> +  at all. Linux 4.1 updated them to implement volatile
> +  semantics via ACCESS_ONCE (or the more recent READ/WRITE_ONCE).
> +
> +  QEMU's atomic_read/set implement, if the compiler supports it, C11
> +  atomic relaxed semantics, and volatile semantics otherwise.

Reads better as:

QEMU's atomic_read/set implement C11 atomic relaxed semantics if the 
compiler supports it, and volatile semantics otherwise.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org