Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752010AbaJTQAW (ORCPT ); Mon, 20 Oct 2014 12:00:22 -0400 Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:46105 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751934AbaJTQAT (ORCPT ); Mon, 20 Oct 2014 12:00:19 -0400 Date: Mon, 20 Oct 2014 17:00:09 +0100 From: Catalin Marinas To: Linus Torvalds Cc: Thomas Gleixner , Davidlohr Bueso , Linux Kernel Mailing List , Matteo Franchin , Darren Hart , Peter Zijlstra , Ingo Molnar , "Paul E. McKenney" , Mike Galbraith Subject: Re: [PATCH] futex: Ensure get_futex_key_refs() always implies a barrier Message-ID: <20141020160009.GK23751@e104818-lin.cambridge.arm.com> References: <1413563929-2664-1-git-send-email-catalin.marinas@arm.com> <1413617580.29249.9.camel@linux-t7sj.site> <1413662314.17869.11.camel@linux-t7sj.site> <1413684978.17869.18.camel@linux-t7sj.site> <20141020104926.GD23751@e104818-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 On Mon, Oct 20, 2014 at 04:32:00PM +0100, Linus Torvalds wrote: > On Mon, Oct 20, 2014 at 3:49 AM, Catalin Marinas > wrote: > > Since you mention symmetry, something like below makes the barriers more > > explicit. > > Borken, for two reasons: > > > diff --git a/kernel/futex.c b/kernel/futex.c > > index f3a3a071283c..5b9d857d0816 100644 > > --- a/kernel/futex.c > > +++ b/kernel/futex.c > > @@ -143,9 +143,7 @@ > > static inline void futex_get_mm(union futex_key *key) > > { > > atomic_inc(&key->private.mm->mm_count); > > - /* > > - * Ensure futex_get_mm() implies a full barrier such that > > - * get_futex_key() implies a full barrier. This is relied upon > > - * as full barrier (B), see the ordering comment above. > > - */ > > - smp_mb__after_atomic(); > > } > > So the thing is, this means that we can't take advantage of the fact > that "atomic_inc" is already an atomic. So this is just a performance > breakage. But: OK, I looked at this from an ARM perspective only and it would not make any difference. But it seems that MIPS makes a distinction between "before" and "after" barriers with "before" defined as wmb in some configuration, so the hunk below would break it. > > static inline int hb_waiters_pending(struct futex_hash_bucket *hb) > > { > > + /* > > + * Full barrier (B), see the ordering comment above. > > + */ > > + smp_mb__before_atomic(); > > #ifdef CONFIG_SMP > > return atomic_read(&hb->waiters); > > This is just entirely broken. > > "atomic_read()" isn't really an "atomic op" at all. despite the name, > it's just a read that is basically ACCESS_ONCE. > > So smp_mb__before_atomic() doesn't work for atomic_read(), and the > code is nonsensical and doesn't work. It would need to be a full > memory barrier. Looking at the semantics of smp_mb__*_atomic(), it would indeed have to be a full smp_mb() here. -- Catalin -- 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/