[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 1/2] add a header file for atomic operations

From: Richard Henderson
Subject: Re: [Qemu-devel] [PATCH v2 1/2] add a header file for atomic operations
Date: Tue, 18 Jun 2013 07:38:26 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6

On 06/18/2013 01:36 AM, Paolo Bonzini wrote:
>> Why all the ifdefs?  If __atomic support is present, then __ATOMIC defines 
>> will
>> exist.
> Then I can just use "#ifdef __ATOMIC_RELAXED" instead of
> "#if QEMU_GNUC_PREREQ(4, 8)"?

I'd say so.

> I have no idea, but I can say which semantics I want:
> 1) Linux kernel memory barrier semantics for smp_*mb (i.e. express the 
> barriers
> in terms of read/write/full, not in terms of acq/rel/seqcst);
> 2) Java volatile semantics for atomic_mb_*.
> Basically, I cannot claim I understand this stuff 100%, but at least I could
> use sources I trust to implement it.

Fair enough.  Excellent pointers to have in the documentation, anyhow.

>>> +#ifndef atomic_read
>>> +#define atomic_read(ptr)       (*(__typeof__(*ptr) *volatile) (ptr))
>>>  #endif
>>> +#ifndef atomic_set
>>> +#define atomic_set(ptr, i)     ((*(__typeof__(*ptr) *volatile) (ptr)) = 
>>> (i))
>>> +#endif
>> Use
>> __atomic_load(..., __ATOMIC_RELAXED)
>> __atomic_store(..., __ATOMIC_RELAXED)
>> ?
> Same here, I didn't want proliferation of #ifdefs beyond what is actually 
> required.

Not knowing exactly where these might be used within the code base, I'd be
worried about someone applying them to a uint64_t, somewhere a 32-bit host
might see it.  At which point the above is going to be silently wrong, loaded
with two 32-bit pieces.

Given that we're not requiring gcc 4.8, and cannot guarantee use of
__atomic_load, perhaps we ought to do something like

#define atomic_read(ptr) \
  ({ if (sizeof(*(ptr)) > sizeof(ptr)) invalid_atomic_read(); \
     *(__typeof__(*ptr) *volatile) (ptr)); })

which should generate a link error when reading a size we can't guarantee will
Just Work.

> The FAQ also has an "important note", however:
>     Important Note: Note that it is important for both threads to access
>     the same volatile variable in order to properly set up the happens-before
>     relationship. 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 release and acquire have to "match" (i.e.,
>     be performed on the same volatile field) to have the right semantics. 
> Is this final "important note" the difference between ACQ_REL and SEQ_CST?

Yes, exactly.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]