Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752997AbbHGLky (ORCPT ); Fri, 7 Aug 2015 07:40:54 -0400 Received: from casper.infradead.org ([85.118.1.10]:49665 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752698AbbHGLkw (ORCPT ); Fri, 7 Aug 2015 07:40:52 -0400 Date: Fri, 7 Aug 2015 13:40:47 +0200 From: Peter Zijlstra To: Vineet Gupta Cc: Thomas Gleixner , Michel Lespinasse , arc-linux-dev@synopsys.com, lkml , David Hildenbrand , Will Deacon , ralf@linux-mips.org, ddaney@caviumnetworks.com Subject: Re: [PATCH 1/4] ARC: add barriers to futex code Message-ID: <20150807114047.GD18673@twins.programming.kicks-ass.net> References: <1438864523-31340-1-git-send-email-vgupta@synopsys.com> <1438864523-31340-2-git-send-email-vgupta@synopsys.com> <20150806134826.GF25159@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150806134826.GF25159@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3009 Lines: 88 On Thu, Aug 06, 2015 at 03:48:26PM +0200, Peter Zijlstra wrote: > On Thu, Aug 06, 2015 at 06:05:20PM +0530, Vineet Gupta wrote: > > The atomic ops on futex need to provide the full barrier just like > > regular atomics in kernel. > > > > Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic() > > as core code already does that > > Urgh, and of course tglx just left for holidays :-) > > > +++ b/arch/arc/include/asm/futex.h > > @@ -20,6 +20,7 @@ > > > > #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\ > > \ > > + smp_mb(); \ > > __asm__ __volatile__( \ > > "1: llock %1, [%2] \n" \ > > insn "\n" \ > > @@ -40,12 +41,14 @@ > > \ > > : "=&r" (ret), "=&r" (oldval) \ > > : "r" (uaddr), "r" (oparg), "ir" (-EFAULT) \ > > - : "cc", "memory") > > + : "cc", "memory"); \ > > + smp_mb(); \ > > > > > So: > > - alhpa: only has the first smp_mb(), suggesting RELEASE > - arm: only has the first smp_mb(), suggesting RELEASE > - arm64: has store-release + smp_mb(), suggesting full barriers > - MIPS: has LLSC_MB after, suggesting ACQUIRE > - powerpc: lwsync before, sync after, full barrier > > x86 is of course boring and fully ordered > > Looking at the usage site of futex_atomic_op_inuser(), that's in > futex_wake_op() which might suggest RELEASE is indeed sufficient. > > Which leaves me puzzled on MIPS, but what do I know. So I _think_ the MIPS code is broken. The mips futex code is from 2006 and the mips smp_mb__before_llsc bits are from 2010, so its well possible this was missed. Ralf, David, did I miss the obvious or does the below patch make sense? --- Subject: MIPS: Fix __futex_atomic_op() for WEAK_REORDERING_BEYOND_LLSC Current __futex_atomic_op() doesn't have smp_mb__before_llsc(), meaning it would allow prior load/stores to escape out and re-order against the ll/sc itself. While the exact requirements on __futex_atomic_op() are a little unclear at the moment it must be either RELEASE or fully ordered, without smp_mb__before_llsc() the MIPS code is neither. Therefore add smp_mb__before_llsc(). Cc: Ralf Baechle Cc: David Daney Signed-off-by: Peter Zijlstra (Intel) --- arch/mips/include/asm/futex.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/mips/include/asm/futex.h b/arch/mips/include/asm/futex.h index 1de190bdfb9c..2b8023b9b661 100644 --- a/arch/mips/include/asm/futex.h +++ b/arch/mips/include/asm/futex.h @@ -20,6 +20,8 @@ #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg) \ { \ + smp_mb__before_llsc(); \ + \ if (cpu_has_llsc && R10000_LLSC_WAR) { \ __asm__ __volatile__( \ " .set push \n" \ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/