From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53356) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoxSI-0005PX-Jn for qemu-devel@nongnu.org; Tue, 18 Jun 2013 11:04:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoxSG-00047K-V8 for qemu-devel@nongnu.org; Tue, 18 Jun 2013 11:04:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62642) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoxSG-00046V-MR for qemu-devel@nongnu.org; Tue, 18 Jun 2013 11:04:20 -0400 Message-ID: <51C076ED.8080501@redhat.com> Date: Tue, 18 Jun 2013 17:04:13 +0200 From: Paolo Bonzini 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> <51C070E2.4050207@twiddle.net> In-Reply-To: <51C070E2.4050207@twiddle.net> 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: Richard Henderson Cc: Liu Ping Fan , Anthony Liguori , qemu-devel@nongnu.org Il 18/06/2013 16:38, Richard Henderson ha scritto: >>>> +#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. Good idea. >> 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. I still have some confusion, so I sent another follow-up with the exact differences in the generated code. But in any case, I will augment the documentation with the text from the FAQ, seems safe. Paolo