Re: [Qemu-devel] [PATCH v1 3/5] include/qemu/atomic.h: default to __atomic functions

[Paolo Bonzini]

On 29/01/2016 17:06, Alex Bennée wrote:
> 
> Paolo Bonzini <address@hidden> writes:
> 
>> On 28/01/2016 11:15, Alex Bennée wrote:
>>> +/* atomic_mb_read/set semantics map Java volatile variables. They are
>>> + * less expensive on some platforms (notably POWER & ARM) than fully
>>> + * sequentially consistent operations.
>>> + *
>>> + * As long as they are used as paired operations they are safe to
>>> + * use. See docs/atomic.txt for more discussion.
>>> + */
> 
> The original comment mentioned both POWER and ARM so I wondering if we
> should also special case for the ARMv7?

I don't know the exact feature test, and I think ARMv7's penalty is much
less because processors are slower, with a less deep pipeline and
usually not used in SMP configurations.

In fact, because it doesn't have "dmb st" and friends, the generated
code should be exactly the same for ARMv7.  Looking at
https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html confirms this.

> I think we are OK because if cmpxchg succeeds _old was by definition
> what was already there but it is confusing and leads to funny code like
> this:
> 
>     if (atomic_cmpxchg(&data[i].n, 0, 3) == 0) {
>         data[i].ret = -ECANCELED;
>         ...
> 
> and
> 
>     if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) {
>        ...
> 
> Which might be easier to read if atomic_cmpxchg used the bool
> semantics, i.e. return true for a successful cmpxchg.

It depends.  When s->state is bool, the bool version is *very* hard to
read.  Then you have two bools and you never know which one it is.  For
example if the expected value is false, atomic_bool_cmpxchg will return
true if the memory location was false...  Aargh! :D

> The old code even has a atomic_bool_cmpxchg which no one seems to use.

Alvise added it, but it's not upstream.

> I wonder if the correct solution is to convert atomic_cmpxchg calls to use
> atomic_cmpxchg_bool calls and remove atomic_cmpxchg from atomic.h?

Yeah, though there are also cases where atomic_cmpxchg_bool is less
efficient.

Not to mention that I don't like the name...  Perhaps atomic_cmpxchg
should be the bool one and atomic_fetch_cmpxchg should return the value.
 Separate patch series though.

Paolo