From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752654AbcBEJov (ORCPT ); Fri, 5 Feb 2016 04:44:51 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33307 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750847AbcBEJoq (ORCPT ); Fri, 5 Feb 2016 04:44:46 -0500 Date: Fri, 5 Feb 2016 10:44:42 +0100 From: Ingo Molnar To: Thomas Gleixner Cc: Davidlohr Bueso , Mel Gorman , Peter Zijlstra , Sebastian Andrzej Siewior , Chris Mason , Darren Hart , Hugh Dickins , linux-kernel@vger.kernel.org, Davidlohr Bueso , Mel Gorman Subject: Re: [PATCH v5] futex: Remove requirement for lock_page in get_futex_key Message-ID: <20160205094441.GB7551@gmail.com> References: <1454567326-16114-1-git-send-email-dave@stgolabs.net> <20160204174343.GA12375@linux-uzut.site> 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 * Thomas Gleixner wrote: > On Thu, 4 Feb 2016, Davidlohr Bueso wrote: > > > On Thu, 04 Feb 2016, Thomas Gleixner wrote: > > > > > On Wed, 3 Feb 2016, Davidlohr Bueso wrote: > > > > + * We are not calling into get_futex_key_refs() in file-backed > > > > + * cases, therefore a successful atomic_inc return below will > > > > + * guarantee that get_futex_key() will continue to imply MB > > > > (B). > > > > > > Can you please make that "MB (B)" part a bit more outstanding. I really had > > > to > > > search for it. > > > > Hmm as you know this is mostly explained at the begining of the file, and we > > sprinkle MB (B) around the code based on that description. So I'm a bit > > confused > > as to why you don't like like that comment. > > The other "MB (B)" places are more outstanding. It did not spring in my eye > immideately. So it's a pure cosmetic issue. So I too didn't understand that sentence at first, because the capitalization really throws off quick parsing of that comment, as 'MB' ususally denotes megabytes. So please change it to "mb(); (A)" or so - and I think all of these comments should be changed to use a standard API name for the barrier they imply, as the head of futex.c does: * waiters++; (a) * mb(); (A) <-- paired with -. * | * lock(hash_bucket(futex)); | * | * uval = *futex; | * | *futex = newval; * | sys_futex(WAKE, futex); * | futex_wake(futex); * | * `-------> mb(); (B) Btw., pedantic: shouldn't that be smp_mb()? Futexes don't operate on IO spaces, so on UP they only need compiler barriers. Thanks, Ingo