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 S932872AbcA2Sqb (ORCPT <rfc822;w@1wt.eu>);
	Fri, 29 Jan 2016 13:46:31 -0500
Received: from mail-pa0-f44.google.com ([209.85.220.44]:33874 "EHLO
	mail-pa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753296AbcA2Sqa (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 29 Jan 2016 13:46:30 -0500
Date: Fri, 29 Jan 2016 10:46:22 -0800 (PST)
From: Hugh Dickins <hughd@google.com>
X-X-Sender: hugh@eggly.anvils
To: Davidlohr Bueso <dave@stgolabs.net>
cc: Hugh Dickins <hughd@google.com>, Davidlohr Bueso <dbueso@suse.de>,
        Mel Gorman <mgorman@techsingularity.net>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Chris Mason <clm@fb.com>, Darren Hart <dvhart@linux.intel.com>,
        linux-kernel@vger.kernel.org, Mel Gorman <mgorman@suse.de>
Subject: Re: [PATCH v4] futex: Remove requirement for lock_page in
 get_futex_key
In-Reply-To: <20160129183557.GA16147@linux-uzut.site>
Message-ID: <alpine.LSU.2.11.1601291043030.4784@eggly.anvils>
References: <1453569254-25928-1-git-send-email-dbueso@suse.de> <alpine.LSU.2.11.1601271815460.1855@eggly.anvils> <20160129183557.GA16147@linux-uzut.site>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 29 Jan 2016, Davidlohr Bueso wrote:
> On Wed, 27 Jan 2016, Hugh Dickins wrote:
> 
> > > +		 *
> > > +		 * The RCU read lock is taken as the inode is finally freed
> > > +		 * under RCU. If the mapping still matches expectations then
> > > the
> > > +		 * mapping->host can be safely accessed as being a valid
> > > inode.
> > > +		 */
> > > +		rcu_read_lock();
> > > +		if (READ_ONCE(page->mapping) != mapping ||
> > > +		    !mapping->host) {
> > 
> > If you're being as paranoid as all the WARN_ON_ONCEs hereabouts imply,
> > then it would be better to do the inode = READ_ONCE(mapping->host)
> > before checking !inode rather than !mapping->host.
> 
> Ok, it also reads a bit nicer than the above, which was simply avoiding
> a load in the again case.
> 
>                rcu_read_lock();
>                inode = READ_ONCE(mapping->host);

Just a quick unthinking from-the-hip response to that, something
for you to think over before sending v5: without looking back
at the code, it's not obvious to me that it's safe to read
mapping->host before we've confirmed under rcu_read_lock()
that page->mapping still matches mapping.

> 
>                if (!inode || READ_ONCE(page->mapping) != mapping)
>                        rcu_read_unlock();
>                        put_page(page);
> 
>                        goto again;
>                }