From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52214) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aPBYq-0004DY-K2 for qemu-devel@nongnu.org; Fri, 29 Jan 2016 11:06:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aPBYl-00074W-IF for qemu-devel@nongnu.org; Fri, 29 Jan 2016 11:06:12 -0500 Received: from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229]:33196) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aPBYk-00074K-TB for qemu-devel@nongnu.org; Fri, 29 Jan 2016 11:06:07 -0500 Received: by mail-wm0-x229.google.com with SMTP id l66so73429792wml.0 for ; Fri, 29 Jan 2016 08:06:06 -0800 (PST) References: <1453976119-24372-1-git-send-email-alex.bennee@linaro.org> <1453976119-24372-4-git-send-email-alex.bennee@linaro.org> <56A9F4BA.1030508@redhat.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <56A9F4BA.1030508@redhat.com> Date: Fri, 29 Jan 2016 16:06:04 +0000 Message-ID: <87si1ggykz.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v1 3/5] include/qemu/atomic.h: default to __atomic functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: mttcg@greensocs.com, peter.maydell@linaro.org, mark.burton@greensocs.com, a.rigo@virtualopensystems.com, qemu-devel@nongnu.org, stefanha@redhat.com, fred.konrad@greensocs.com Paolo Bonzini writes: > On 28/01/2016 11:15, Alex Bennée wrote: >> +/* atomic_mb_read/set semantics map Java volatile variables. They are >> + * less expensive on some platforms (notably POWER & ARM) than fully >> + * sequentially consistent operations. >> + * >> + * As long as they are used as paired operations they are safe to >> + * use. See docs/atomic.txt for more discussion. >> + */ >> + >> +#define atomic_mb_read(ptr) \ >> + ({ \ >> + typeof(*ptr) _val; \ >> + __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \ >> + smp_rmb(); \ >> + _val; \ >> + }) >> + >> +#define atomic_mb_set(ptr, i) do { \ >> + typeof(*ptr) _val = (i); \ >> + smp_wmb(); \ >> + __atomic_store(ptr, &_val, __ATOMIC_RELAXED); \ >> + smp_mb(); \ >> +} while(0) > > Great... I'll change this to > > #if defined(_ARCH_PPC) > #define atomic_mb_read(ptr) \ > ({ \ > typeof(*ptr) _val; \ > __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \ > smp_rmb(); \ > _val; \ > }) > > #define atomic_mb_set(ptr, i) do { \ > typeof(*ptr) _val = (i); \ > smp_wmb(); \ > __atomic_store(ptr, &_val, __ATOMIC_RELAXED); \ > smp_mb(); \ > } while(0) > #else > #define atomic_mb_read(ptr) \ > ({ \ > typeof(*ptr) _val; \ > __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST); \ > _val; \ > }) > > #define atomic_mb_set(ptr, i) do { \ > typeof(*ptr) _val = (i); \ > __atomic_store(ptr, &_val, __ATOMIC_SEQ_CST); \ > } while(0) > #endif > > since this benefits x86 (which can generate mov/xchg respectively) and > aarch64 (where atomic_mb_read/atomic_mb_set map directly to > ldar/stlr). The original comment mentioned both POWER and ARM so I wondering if we should also special case for the ARMv7? > >> +/* Returns the eventual value, failed or not */ Yeah this comment in bogus. >> +#define atomic_cmpxchg(ptr, old, new) \ >> + ({ \ >> + typeof(*ptr) _old = (old), _new = (new); \ >> + __atomic_compare_exchange(ptr, &_old, &_new, false, \ >> + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \ >> + _old; /* can this race if cmpxchg not used elsewhere? */ \ >> + }) > > How so? My mistake, I was having a worry that we weren't following the old semantics. In fact having read even more closely I understand that _old is updated by the __atomic function if the update fails. In fact _old is a poor name because its _expected at the start and _current in the case it fails. In fact: This compares the contents of *ptr with the contents of *expected. If equal, the operation is a read-modify-write operation that writes desired into *ptr. If they are not equal, the operation is a read and the current contents of *ptr are written into *expected. ... If desired is written into *ptr then true is returned and memory is affected according to the memory order specified by success_memorder. There are no restrictions on what memory order can be used here. I was wondering if this was subtly different from the old __sync_val_compare_and_swap: The “val” version returns the contents of *ptr before the operation. I think we are OK because if cmpxchg succeeds _old was by definition what was already there but it is confusing and leads to funny code like this: if (atomic_cmpxchg(&data[i].n, 0, 3) == 0) { data[i].ret = -ECANCELED; ... and if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) { ... Which might be easier to read if atomic_cmpxchg used the bool semantics, i.e. return true for a successful cmpxchg. The old code even has a atomic_bool_cmpxchg which no one seems to use. I wonder if the correct solution is to convert atomic_cmpxchg calls to use atomic_cmpxchg_bool calls and remove atomic_cmpxchg from atomic.h? What do you think? > > Paolo -- Alex Bennée