From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoecJ-0005pV-LC for qemu-devel@nongnu.org; Mon, 17 Jun 2013 14:57:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoecG-0004dp-8K for qemu-devel@nongnu.org; Mon, 17 Jun 2013 14:57:27 -0400 Received: from mail-ye0-f179.google.com ([209.85.213.179]:46160) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoecG-0004d8-2G for qemu-devel@nongnu.org; Mon, 17 Jun 2013 14:57:24 -0400 Received: by mail-ye0-f179.google.com with SMTP id r3so1041623yen.38 for ; Mon, 17 Jun 2013 11:57:23 -0700 (PDT) Sender: Richard Henderson Message-ID: <51BF5C0F.6020209@twiddle.net> Date: Mon, 17 Jun 2013 11:57:19 -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> In-Reply-To: <1371381681-14252-2-git-send-email-pingfanl@linux.vnet.ibm.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: Liu Ping Fan Cc: Paolo Bonzini , qemu-devel@nongnu.org, Anthony Liguori On 06/16/2013 04:21 AM, Liu Ping Fan wrote: > +#if QEMU_GNUC_PREREQ(4, 8) > +#ifndef __ATOMIC_RELAXED > +#define __ATOMIC_RELAXED 0 > +#endif Why all the ifdefs? If __atomic support is present, then __ATOMIC defines will exist. > +/* Compiler barrier */ > +#define barrier() ({ asm volatile("" ::: "memory"); (void)0; }) > + > +#if !QEMU_GNUC_PREREQ(4, 8) With C++11 atomic support we should define barrier as __atomic_signal_fence(__ATOMIC_ACQ_REL) > #if defined(__powerpc64__) > -#define smp_rmb() asm volatile("lwsync" ::: "memory") > +#define smp_rmb() ({ asm volatile("lwsync" ::: "memory"); (void)0; }) > #else > -#define smp_rmb() asm volatile("sync" ::: "memory") > +#define smp_rmb() ({ asm volatile("sync" ::: "memory"); (void)0; }) > #endif Err... lwsync corresponds to ACQ_REL consistency, while sync corresponds to SEQ_CST consistency. The newly added document says you want SEQ_CST while the old code implies ACQ_REL. Which is true? > +#ifndef smp_mb > +#define smp_mb() __sync_synchronize() > +#endif Use __atomic_thread_fence here for completeness? > +#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) ? > + * And for the few ia64 lovers that exist, an atomic_mb_read is a ld.acq, > + * while an atomic_mb_set is a st.rel followed by a memory barrier. ... > + */ > +#ifndef atomic_mb_read > +#if QEMU_GNUC_PREREQ(4, 8) > +#define atomic_mb_read(ptr) ({ \ > + typeof(*ptr) _val; \ > + __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST); \ > + _val; \ > +}) > +#else > +#define atomic_mb_read(ptr) ({ \ > + typeof(*ptr) _val = atomic_read(ptr); \ > + smp_rmb(); \ > + _val; \ This latter definition is ACQUIRE not SEQ_CST (except for ia64). Without load_acquire, one needs barriers before and after the atomic_read in order to implement SEQ_CST. So again I have to ask, what semantics are you actually looking for here? > +#define atomic_mb_set(ptr, i) do { \ > + smp_wmb(); \ > + atomic_set(ptr, i); \ > + smp_mb(); \ > +} while (0) Really only a smp_wmb? What about outstanding loads? r~