From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754097AbcFPKQk (ORCPT ); Thu, 16 Jun 2016 06:16:40 -0400 Received: from foss.arm.com ([217.140.101.70]:42683 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752587AbcFPKQg (ORCPT ); Thu, 16 Jun 2016 06:16:36 -0400 Date: Thu, 16 Jun 2016 11:16:28 +0100 From: Will Deacon To: Boqun Feng Cc: Waiman Long , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, x86@kernel.org, linux-alpha@vger.kernel.org, linux-ia64@vger.kernel.org, linux-s390@vger.kernel.org, linux-arch@vger.kernel.org, Davidlohr Bueso , Jason Low , Dave Chinner , Scott J Norton , Douglas Hatch Subject: Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier Message-ID: <20160616101627.GA5827@arm.com> References: <1465944489-43440-1-git-send-email-Waiman.Long@hpe.com> <1465944489-43440-2-git-send-email-Waiman.Long@hpe.com> <20160615080446.GA28443@insomnia> <5761A5FF.5070703@hpe.com> <20160616021951.GA16918@insomnia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160616021951.GA16918@insomnia> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi guys, On Thu, Jun 16, 2016 at 10:19:51AM +0800, Boqun Feng wrote: > On Wed, Jun 15, 2016 at 03:01:19PM -0400, Waiman Long wrote: > > On 06/15/2016 04:04 AM, Boqun Feng wrote: > > > On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote: > > > > @@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock) > > > > * Second most likely case. > > > > */ > > > > node = this_cpu_ptr(&osq_node); > > > > - next = xchg(&node->next, NULL); > > > > + next = xchg_release(&node->next, NULL); > > > > if (next) { > > > > WRITE_ONCE(next->locked, 1); > > > So we still use WRITE_ONCE() rather than smp_store_release() here? > > > > > > Though, IIUC, This is fine for all the archs but ARM64, because there > > > will always be a xchg_release()/xchg() before the WRITE_ONCE(), which > > > carries a necessary barrier to upgrade WRITE_ONCE() to a RELEASE. > > > > > > Not sure whether it's a problem on ARM64, but I think we certainly need > > > to add some comments here, if we count on this trick. > > > > > > Am I missing something or misunderstanding you here? > > > > > The change on the unlock side is more for documentation purpose than is > > actually needed. As you had said, the xchg() call has provided the necessary > > memory barrier. Using the _release variant, however, may have some > > But I'm afraid the barrier doesn't remain if we replace xchg() with > xchg_release() on ARM64v8, IIUC, xchg_release() is just a ldxr+stlxr > loop with no barrier on ARM64v8. This means the following code: > > CPU 0 CPU 1 (next) > ======================== ================== > WRITE_ONCE(x, 1); r1 = smp_load_acquire(next->locked, 1); > xchg_release(&node->next, NULL); r2 = READ_ONCE(x); > WRITE_ONCE(next->locked, 1); > > could result in (r1 == 1 && r2 == 0) on ARM64v8, IIUC. Yes, of course. Why is that unexpected? You could just as easily make the xchg_release an smp_store_release and this would still be permitted, that's the whole point of acquire/release -- they're semi-permeable barriers that allow accesses outside of the critical section to leak in, but not the other way around. It's worth noting that you've omitted the control dependency from xchg_release to the subsequent write in your litmus tests, but I don't think that actually changes anything here. Will