Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752773AbbHFN2O (ORCPT ); Thu, 6 Aug 2015 09:28:14 -0400 Received: from us01smtprelay-2.synopsys.com ([198.182.60.111]:34471 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750758AbbHFN2N convert rfc822-to-8bit (ORCPT ); Thu, 6 Aug 2015 09:28:13 -0400 From: Vineet Gupta To: David Hildenbrand CC: "Peter Zijlstra (Intel)" , Thomas Gleixner , Michel Lespinasse , "arc-linux-dev@synopsys.com" , lkml Subject: Re: [PATCH 1/4] ARC: add barriers to futex code Thread-Topic: [PATCH 1/4] ARC: add barriers to futex code Thread-Index: AQHQ0ERwtxHZIrscCESTiHxoDBq0qw== Date: Thu, 6 Aug 2015 13:28:08 +0000 Message-ID: References: <1438864523-31340-1-git-send-email-vgupta@synopsys.com> <1438864523-31340-2-git-send-email-vgupta@synopsys.com> <20150806151545.0f5f3845@thinkpad-w530> Accept-Language: en-US, en-IN Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.12.197.191] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3143 Lines: 113 On Thursday 06 August 2015 06:45 PM, David Hildenbrand 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 >> >> Cc: David Hildenbrand >> Cc: Peter Zijlstra (Intel) >> Cc: Thomas Gleixner >> Cc: Michel Lespinasse >> Signed-off-by: Vineet Gupta >> --- >> arch/arc/include/asm/futex.h | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h >> index 70cfe16b742d..160656d0a15a 100644 >> --- a/arch/arc/include/asm/futex.h >> +++ 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(); \ > I think you should drop the ; OK sure ! > >> #else /* !CONFIG_ARC_HAS_LLSC */ >> >> #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\ >> \ >> + smp_mb(); \ >> __asm__ __volatile__( \ >> "1: ld %1, [%2] \n" \ >> insn "\n" \ >> @@ -65,7 +68,8 @@ >> \ >> : "=&r" (ret), "=&r" (oldval) \ >> : "r" (uaddr), "r" (oparg), "ir" (-EFAULT) \ >> - : "cc", "memory") >> + : "cc", "memory"); \ >> + smp_mb(); \ > dito OK ! > >> #endif >> >> @@ -151,7 +155,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval, >> if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int))) >> return -EFAULT; >> >> - pagefault_disable(); >> + smp_mb(); >> >> __asm__ __volatile__( >> #ifdef CONFIG_ARC_HAS_LLSC >> @@ -178,7 +182,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval, >> : "r"(oldval), "r"(newval), "r"(uaddr), "ir"(-EFAULT) >> : "cc", "memory"); >> >> - pagefault_enable(); >> + smp_mb(); >> >> *uval = val; >> return val; > Looks like pagefault_() magic is only required for futex_atomic_op_inuser. So > this should be fine (and arc seems to be the only arch left that has in in > _inatomic). > > Not sure if you want to change the comment: > > /* Compare-xchg with pagefaults disabled. > * Notes: > * -Best-Effort: Exchg happens only if compare succeeds. > > Maybe something like "Compare-xchg: pagefaults have to be disabled by the > caller" Will do ! > Looks sane to me. Thx for the quick review David. It seems ARC also needs the preempt disable magic for !LLSC config. I'll send a separate patch to that effect. Thx, -Vineet > > David > > -- 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/