From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755113AbcBESCt (ORCPT <rfc822;w@1wt.eu>);
	Fri, 5 Feb 2016 13:02:49 -0500
Received: from mx2.suse.de ([195.135.220.15]:38163 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753109AbcBESCs (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 5 Feb 2016 13:02:48 -0500
Date: Fri, 5 Feb 2016 10:02:39 -0800
From: Davidlohr Bueso <dave@stgolabs.net>
To: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
        Mel Gorman <mgorman@techsingularity.net>,
        Peter Zijlstra <peterz@infradead.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Chris Mason <clm@fb.com>, Darren Hart <dvhart@linux.intel.com>,
        Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org,
        Davidlohr Bueso <dbueso@suse.de>, Mel Gorman <mgorman@suse.de>
Subject: Re: [PATCH v5] futex: Remove requirement for lock_page in
 get_futex_key
Message-ID: <20160205180239.GB12375@linux-uzut.site>
References: <1454567326-16114-1-git-send-email-dave@stgolabs.net>
 <alpine.DEB.2.11.1602041002010.25254@nanos>
 <20160204174343.GA12375@linux-uzut.site>
 <alpine.DEB.2.11.1602041845280.25254@nanos>
 <20160205094441.GB7551@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20160205094441.GB7551@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 05 Feb 2016, Ingo Molnar wrote:

>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.

Sure, fair enough.

>
>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.

Right, but we do in fact use smp barriers in this cases in the real code, that mb() is
just in the comments, I guess it would be desirable to change it to smp_mb nonetheless.

However, could these changes be in a followup? Mainly because the barrier B references
will be updated across all futex.c... unless there are still concerns about this particular
patch, of course.

Thanks,
Davidlohr