Re: [Qemu-devel] [PATCH V3 01/11] atomic: introduce atomic operations

[Jamie Lokier]

Avi Kivity wrote:
> On 09/13/2012 09:54 AM, liu ping fan wrote:
> 
> >>> +typedef struct Atomic {
> >>> +    int counter;
> >>> +} Atomic;
> >>
> >> Best to mark counter 'volatile'.
> >>
> >>> +
> >>> +static inline void atomic_set(Atomic *v, int i)
> >>> +{
> >>> +    v->counter = i;
> >>> +}
> >>> +
> >>> +static inline int atomic_read(Atomic *v)
> >>> +{
> >>> +    return v->counter;
> >>> +}
> >>>
> >>
> >> So these two operations don't get mangled by the optimizer.
> >>
> > Browsing linux code and reading lkml, find some similar material. But
> > they have moved volatile from ->counter to function - atomic_read().
> > As to atomic_read(), I think it need to prevent optimizer from
> > refetching issue, but as to atomic_set(), do we need ?
> 
> I think so, to prevent reordering.

Hi,

I don't think volatile makes any difference to reordering here.

The compiler is not going to move the atomic_set() store before or
after another instruction on the same atomic variable anyway, just
like it wouldn't do that for an ordinary assignment.

If you're concerned about ordering with respect to other memory, then
volatile wouldn't make much difference.  barrier() before and after would.

If you're copying Linux's semantics, Linux's atomic_set() doesn't
include any barriers, nor imply any.  atomic_read() uses volatile to
ensure that each call re-reads the value, for example in a loop.
(Same as ACCESS_ONCE()).  If there was a call to atomic_set() in a
loop, it doesn't guarantee that will be written each time.

-- Jamie