From patchwork Mon Oct 20 10:49:26 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Catalin Marinas X-Patchwork-Id: 39044 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-lb0-f199.google.com (mail-lb0-f199.google.com [209.85.217.199]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id AA493203C5 for ; Mon, 20 Oct 2014 10:49:45 +0000 (UTC) Received: by mail-lb0-f199.google.com with SMTP id w7sf2329066lbi.2 for ; Mon, 20 Oct 2014 03:49:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:cc:subject:message-id :references:mime-version:in-reply-to:user-agent:sender:precedence :list-id:x-original-sender:x-original-authentication-results :mailing-list:list-post:list-help:list-archive:list-unsubscribe :content-type:content-disposition; bh=4rIsJKz7PYeOJ7QgHrD5hMeyoTo8akNyQ1hxQTuSodI=; b=BZFPd0odx0si1C5fCQSpLn/WWFZukzY60Kn58jkfeF5AcGORNMVmfAUrP1LrsPJF/6 eOuU3p9/Jq0shPNWmXGg7dD+dVVUoK7IdrwX23fKYYENOfHqVRb9wHbBTWjiJ/iaqmiM fH7VcFkTR7Oy0IujdaDjGQwb+jbVXPlDlC1iq4TWhWnhSWokVxKThfSyQLcGqI29d3tQ QUnlh1w3PpeVVdgM1Urk5V0Y6ii9Ka75GmNCCniJV8HGSGUiWHsvGASioS515q2W47Qt oahHjWOusDuVVXASJCVEiyzJRgRn9EThoOY18gqpTewqvPlc8JmSnVfpG/OXIUPNbbWW CB8g== X-Gm-Message-State: ALoCoQk8fYfIb/rD2AE8PmATCugMuFWhCP5xpLjtR3qe/ObY173o3KwHbIV7OBRqU2Ho+scCEAFC X-Received: by 10.152.25.202 with SMTP id e10mr3922729lag.2.1413802184404; Mon, 20 Oct 2014 03:49:44 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.10.2 with SMTP id e2ls43390lab.88.gmail; Mon, 20 Oct 2014 03:49:44 -0700 (PDT) X-Received: by 10.152.43.197 with SMTP id y5mr8285323lal.82.1413802184227; Mon, 20 Oct 2014 03:49:44 -0700 (PDT) Received: from mail-la0-f44.google.com (mail-la0-f44.google.com. [209.85.215.44]) by mx.google.com with ESMTPS id ku5si13744735lac.30.2014.10.20.03.49.44 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 20 Oct 2014 03:49:44 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.44 as permitted sender) client-ip=209.85.215.44; Received: by mail-la0-f44.google.com with SMTP id hs14so3706947lab.3 for ; Mon, 20 Oct 2014 03:49:44 -0700 (PDT) X-Received: by 10.112.12.35 with SMTP id v3mr8321863lbb.80.1413802184018; Mon, 20 Oct 2014 03:49:44 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.84.229 with SMTP id c5csp279012lbz; Mon, 20 Oct 2014 03:49:43 -0700 (PDT) X-Received: by 10.68.65.7 with SMTP id t7mr4685629pbs.1.1413802182316; Mon, 20 Oct 2014 03:49:42 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n7si7533543pdp.72.2014.10.20.03.49.41 for ; Mon, 20 Oct 2014 03:49:42 -0700 (PDT) Received-SPF: none (google.com: linux-kernel-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753546AbaJTKti (ORCPT + 27 others); Mon, 20 Oct 2014 06:49:38 -0400 Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:45869 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753112AbaJTKth (ORCPT ); Mon, 20 Oct 2014 06:49:37 -0400 Received: from foss-smtp-na-1.foss.arm.com (unknown [10.80.61.8]) by foss-mx-na.foss.arm.com (Postfix) with ESMTP id 715D6EE; Mon, 20 Oct 2014 05:49:32 -0500 (CDT) Received: from collaborate-mta1.arm.com (highbank-bc01-b06.austin.arm.com [10.112.81.134]) by foss-smtp-na-1.foss.arm.com (Postfix) with ESMTP id 218295FAD7; Mon, 20 Oct 2014 05:49:30 -0500 (CDT) Received: from e104818-lin.cambridge.arm.com (e104818-lin.cambridge.arm.com [10.1.203.148]) by collaborate-mta1.arm.com (Postfix) with ESMTPS id 7835F13F697; Mon, 20 Oct 2014 05:49:28 -0500 (CDT) Date: Mon, 20 Oct 2014 11:49:26 +0100 From: Catalin Marinas To: Thomas Gleixner Cc: Davidlohr Bueso , Linus Torvalds , 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: <20141020104926.GD23751@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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: catalin.marinas@arm.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.44 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , Content-Disposition: inline On Mon, Oct 20, 2014 at 10:11:40AM +0100, Thomas Gleixner wrote: > On Sat, 18 Oct 2014, Davidlohr Bueso wrote: > > On Sat, 2014-10-18 at 13:50 -0700, Linus Torvalds wrote: > > > On Sat, Oct 18, 2014 at 12:58 PM, Davidlohr Bueso wrote: > > > > > > > > And [get/put]_futex_keys() shouldn't even be called for private futexes. > > > > The following patch had some very minor testing on a 60 core box last > > > > night, but passes both Darren's and perf's tests. So I *think* this is > > > > right, but lack of sleep and I overall just don't trust them futexes! > > > > > > Hmm. I don't see the advantage of making the code more complex in > > > order to avoid the functions that are no-ops for the !fshared case? > > > > > > IOW, as far as I can tell, this patch doesn't actually really *change* > > > anything. Am I missing something? > > > > Right, all we do is avoid a NOP, but I don't see how this patch makes > > the code more complex. In fact, the whole idea is to make it easier to > > read and makes the key referencing differences between shared and > > private futexes crystal clear, hoping to mitigate future bugs. > > I tend to disagree. The current code is symetric versus get/drop and > you make it unsymetric by avoiding the drop call with a pointless > extra conditional in all call sites. Since you mention symmetry, something like below makes the barriers more explicit. However, it removes the implied barrier for get_futex_key_refs and the other calling places need to be verified (requeue_futex and requeue_pi_futex; if barrier is not required here, the result may be slightly more optimal, not sure you would see the difference though). --- 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/ 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 @@ * * Where (A) orders the waiters increment and the futex value read through * atomic operations (see hb_waiters_inc) and where (B) orders the write - * to futex and the waiters read -- this is done by the barriers in - * get_futex_key_refs(), through either ihold or atomic_inc, depending on the - * futex type. + * to futex and the waiters read (see hb_waiters_pending). * * This yields the following case (where X:=waiters, Y:=futex): * @@ -262,12 +260,6 @@ static struct futex_hash_bucket *futex_queues; 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(); } /* @@ -297,6 +289,10 @@ static inline void hb_waiters_dec(struct futex_hash_bucket *hb) 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); #else @@ -338,13 +334,11 @@ static void get_futex_key_refs(union futex_key *key) switch (key->both.offset & (FUT_OFF_INODE|FUT_OFF_MMSHARED)) { case FUT_OFF_INODE: - ihold(key->shared.inode); /* implies MB (B) */ + __iget(key->shared.inode); break; case FUT_OFF_MMSHARED: - futex_get_mm(key); /* implies MB (B) */ + futex_get_mm(key); break; - default: - smp_mb(); /* explicit MB (B) */ } } @@ -417,7 +411,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw) if (!fshared) { key->private.mm = mm; key->private.address = address; - get_futex_key_refs(key); /* implies MB (B) */ + get_futex_key_refs(key); return 0; } @@ -524,7 +518,7 @@ again: key->shared.pgoff = basepage_index(page); } - get_futex_key_refs(key); /* implies MB (B) */ + get_futex_key_refs(key); out: unlock_page(page_head);