From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49299) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoxH0-0001Dw-R1 for qemu-devel@nongnu.org; Tue, 18 Jun 2013 10:52:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoxGv-0007af-4m for qemu-devel@nongnu.org; Tue, 18 Jun 2013 10:52:42 -0400 Received: from mail-ye0-f173.google.com ([209.85.213.173]:47146) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uox3I-0001Vb-2j for qemu-devel@nongnu.org; Tue, 18 Jun 2013 10:38:32 -0400 Received: by mail-ye0-f173.google.com with SMTP id m7so1375189yen.18 for ; Tue, 18 Jun 2013 07:38:31 -0700 (PDT) Sender: Richard Henderson Message-ID: <51C070E2.4050207@twiddle.net> Date: Tue, 18 Jun 2013 07:38:26 -0700 From: Richard Henderson MIME-Version: 1.0 References: <1371381681-14252-1-git-send-email-pingfanl@linux.vnet.ibm.com> <1371381681-14252-2-git-send-email-pingfanl@linux.vnet.ibm.com> <51BF5C0F.6020209@twiddle.net> <51C01C0E.5030501@redhat.com> In-Reply-To: <51C01C0E.5030501@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] add a header file for atomic operations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Liu Ping Fan , Anthony Liguori , qemu-devel@nongnu.org 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. r~