From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50836) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2qSF-0006kJ-FZ for qemu-devel@nongnu.org; Tue, 17 May 2016 21:39:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b2lTR-0003ce-Ms for qemu-devel@nongnu.org; Tue, 17 May 2016 16:20:17 -0400 Received: from mail-lf0-x244.google.com ([2a00:1450:4010:c07::244]:36569) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2lTR-0003bi-Ex for qemu-devel@nongnu.org; Tue, 17 May 2016 16:20:13 -0400 Received: by mail-lf0-x244.google.com with SMTP id y84so1926153lfc.3 for ; Tue, 17 May 2016 13:20:13 -0700 (PDT) References: <1463196873-17737-1-git-send-email-cota@braap.org> <1463196873-17737-8-git-send-email-cota@braap.org> <573B5134.8060104@gmail.com> <66d14198-dab0-c72e-fe17-d022cff3feff@twiddle.net> <20160517200415.GD30174@flamenco> From: Sergey Fedorov Message-ID: <573B7CFB.30002@gmail.com> Date: Tue, 17 May 2016 23:20:11 +0300 MIME-Version: 1.0 In-Reply-To: <20160517200415.GD30174@flamenco> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 07/18] qemu-thread: add simple test-and-set spinlock List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Emilio G. Cota" , Richard Henderson Cc: QEMU Developers , MTTCG Devel , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Paolo Bonzini , Peter Crosthwaite On 17/05/16 23:04, Emilio G. Cota wrote: > On Tue, May 17, 2016 at 12:19:27 -0700, Richard Henderson wrote: >> On 05/17/2016 10:13 AM, Sergey Fedorov wrote: >>>>> +static inline void qemu_spin_lock(QemuSpin *spin) >>>>> +{ >>>>> + while (atomic_test_and_set_acquire(&spin->value)) { >>> >From gcc-4.8 info page, node "__atomic Builtins", description of >>> __atomic_test_and_set(): >>> >>> It should be only used for operands of type 'bool' or 'char'. >>> >> Hum. I thought I remembered all operand sizes there, but I've just re-checked >> and you're right about bool (and really only bool). >> >> Perhaps we should just stick with __sync_test_and_set then. I'm thinking here >> of e.g. armv6, a reasonable host, which can't operate on 1 byte atomic values. > I like this idea, it gets rid of any guesswork (as in my previous email). > I've changed the patch to: > > commit 8f89d36b6203b78df2bf1e3f82871b8aa2ca83b7 > Author: Emilio G. Cota > Date: Thu Apr 28 10:56:26 2016 -0400 > > atomics: add atomic_test_and_set_acquire > > Signed-off-by: Emilio G. Cota > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index 5bc4d6c..95de7a7 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -113,6 +113,13 @@ > } while(0) > #endif > > +/* > + * We might we tempted to use __atomic_test_and_set with __ATOMIC_ACQUIRE; > + * however, the documentation explicitly says that we should only pass > + * a boolean to it, so we use __sync_lock_test_and_set, which doesn't > + * have this limitation, and is documented to have acquire semantics. > + */ > +#define atomic_test_and_set_acquire(ptr) __sync_lock_test_and_set(ptr, true) So you are going to stick to *legacy* built-ins? Kind regards, Sergey > > /* All the remaining operations are fully sequentially consistent */ > > @@ -327,6 +334,8 @@ > #endif > #endif > > +#define atomic_test_and_set_acquire(ptr) __sync_lock_test_and_set(ptr, true) > + > /* Provide shorter names for GCC atomic builtins. */ > #define atomic_fetch_inc(ptr) __sync_fetch_and_add(ptr, 1) > #define atomic_fetch_dec(ptr) __sync_fetch_and_add(ptr, -1) > --- > > An alternative would be to add just a single line, right below the > barrier() definition or at the end of the file. Adding both lines > is IMO a bit clearer, since the newly-added comment only applies > under the C11 definitions.