Re: [PATCH 1/8] qatomic: add smp_mb__before/after_rmw()

[Richard Henderson]

On 3/3/23 09:19, Paolo Bonzini wrote:
> This nasty difference between Linux and C11 read-modify-write operations
> has already caused issues in util/async.c and more are being found.
> Provide something similar to Linux smp_mb__before/after_atomic(); this
> has the double function of documenting clearly why there is a memory
> barrier, and avoiding a double barrier on x86 and s390x systems.

It does make me question our choice to use neither the Linux nor the C11 model.

> +      +--------------------------------+
> +      | QEMU (incorrect)               |
> +      +================================+
> +      | ::                             |
> +      |                                |
> +      |   a = qatomic_fetch_add(&x, 2);|
> +      |   smp_mb__after_rmw();         |
> +      |   b = qatomic_read(&y);        |
> +      +--------------------------------+

Correct, surely.

> +/*
> + * SEQ_CST is weaker than the older __sync_* builtins and Linux
> + * kernel read-modify-write atomics.  Provide a macro to obtain
> + * the same semantics.
> + */
> +#if !defined(QEMU_SANITIZE_THREAD) && \
> +    (defined(__i386__) || defined(__x86_64__) || defined(__s390x__))
> +# define smp_mb__before_rmw() signal_barrier()
> +# define smp_mb__after_rmw() signal_barrier()
> +#else
> +# define smp_mb__before_rmw() smp_mb()
> +# define smp_mb__after_rmw() smp_mb()
> +#endif

I notice you never actually use smp_mb__before_rmw(), but I suppose since we're trying to 
mirror smp_mb__before(), we should keep the interface whole.

Other than the "incorrect" above,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~