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.
We do use the C11 model. However, C11's "just use seqcst everywhere" is not suitable to define higher-level primitives—including those that are part of the C11 or pthreads specifications.
For example, C11 implementations (such as glibc) have the same issue as QEMU in their implementations of e.g. pthread_cond_wait or pthread_join. The primitive itself could in principle be implemented using seqcst atomics, but it has to assume that the caller uses not even relaxed atomics, but just plain old non-atomic accesses.
If instead you mean using acqrel memory ordering instead of seqcst for the RMW operations, honestly I think I understand acq and rel pretty well at this point but I don't understand acqrel enough to be able to use it.
Paolo
> + +--------------------------------+
> + | 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~