Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753564AbdIDMA1 (ORCPT ); Mon, 4 Sep 2017 08:00:27 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:38835 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753531AbdIDMA0 (ORCPT ); Mon, 4 Sep 2017 08:00:26 -0400 Date: Mon, 4 Sep 2017 14:00:09 +0200 From: Peter Zijlstra To: "Reshetova, Elena" Cc: Thomas Gleixner , "linux-kernel@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" , "gregkh@linuxfoundation.org" , "viro@zeniv.linux.org.uk" , "tj@kernel.org" , "mingo@redhat.com" , "hannes@cmpxchg.org" , "lizefan@huawei.com" , "acme@kernel.org" , "alexander.shishkin@linux.intel.com" , "eparis@redhat.com" , "akpm@linux-foundation.org" , "arnd@arndb.de" , "luto@kernel.org" , "keescook@chromium.org" , "dvhart@infradead.org" , "ebiederm@xmission.com" Subject: Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t Message-ID: <20170904120009.ah2qu3lbgdqdgz6i@hirez.programming.kicks-ass.net> References: <20170901093852.it4d4bxoy2lmojrk@hirez.programming.kicks-ass.net> <2236FBA76BA1254E88B949DDB74E612B6FF6347F@IRSMSX102.ger.corp.intel.com> <20170901123415.s3fxlyeyourz47av@hirez.programming.kicks-ass.net> <2236FBA76BA1254E88B949DDB74E612B6FF63506@IRSMSX102.ger.corp.intel.com> <20170901133644.jf57pwuaep6zirxz@hirez.programming.kicks-ass.net> <2236FBA76BA1254E88B949DDB74E612B6FF6369D@IRSMSX102.ger.corp.intel.com> <20170901191234.ghybbmpm73miwmkp@hirez.programming.kicks-ass.net> <2236FBA76BA1254E88B949DDB74E612B6FF63ED7@IRSMSX102.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2236FBA76BA1254E88B949DDB74E612B6FF63ED7@IRSMSX102.ger.corp.intel.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2439 Lines: 70 On Mon, Sep 04, 2017 at 10:31:54AM +0000, Reshetova, Elena wrote: > > > But can they make "fast" implementation on ARM that would give stronger > > > memory guarantees? > > > > Whatever for? > > Well, maybe just by default when arch.-specific implementation is > done. But I was just trying to speculate to understand. I will resend > this one with new comment added. So the generic lib/refcount.c already has weak ordering. It doesn't make sense for an arch specific implementation (on a weakly ordered machine) to provide stronger guarantees (it would make things slower). The weaker ordering of the refcount_t primitives is sufficient if we're talking pure refcounts. If for some reason code relies on stronger ordering there _SHOULD_ be a comment with describing the additional ordering requirements. But that's a fairly big 'should'. I can well imagine the comment not being there. In fact, see below. > Still not sure if I need to resend the whole series with updated > commits or break this up by individual patches further for the > separate merges. I've yet to look at the ones targeted at subsystems I do, I'm forever and terminally behind on review :/ I called out the issue on futex in particular because it is fairly tricky code that. Now Thomas would like you to mention the fact that refcount_t doesn't provide the exact same ordering as the atomic_t usages it replaces and I think it would be good if you could hand-wave an argument on why the futex code doesn't care. Now, suppose we were to convert i_count to refcount_t (yes, I know, my initial conversion wasn't well received), then we need to add futex_get_inode() similar to futex_get_mm(). That is, smp_mb__{before,after}_atomic() works as expected and can be used to fortify the implied barriers by refcount_t. --- Subject: fs,inode: Add comment explaining additional ordering Add a note to ihold() to document the ordering futex relies upon. Signed-off-by: Peter Zijlstra (Intel) --- fs/inode.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/inode.c b/fs/inode.c index 50370599e371..17192ba92fef 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -395,6 +395,10 @@ void __iget(struct inode *inode) */ void ihold(struct inode *inode) { + /* + * Note: futex.c:get_futex_key_refs() relies on this function + * implying an smp_mb(). + */ WARN_ON(atomic_inc_return(&inode->i_count) < 2); } EXPORT_SYMBOL(ihold);