Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Feb 2022 21:28:49 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/75] mm/gup: Increment the page refcount before the
 pincount
Message-ID: <Yf2akQ8vRj21eDUC@casper.infradead.org>
References: <20220204195852.1751729-1-willy@infradead.org>
 <20220204195852.1751729-2-willy@infradead.org>
 <185cd8a2-8d77-1933-987c-7b1d4f0e8603@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <185cd8a2-8d77-1933-987c-7b1d4f0e8603@nvidia.com>
Precedence: bulk

On Fri, Feb 04, 2022 at 01:13:29PM -0800, John Hubbard wrote:
> On 2/4/22 11:57, Matthew Wilcox (Oracle) wrote:
> > We should always increase the refcount before doing anything else to
> > the page so that other page users see the elevated refcount first.
> 
> Absolutely agree in principle. Is there anything else to say, though,
> such as why this matters here? Or is the change just being done for
> "best practices"? (Which is still a very solid reason, of course.)

I'm not sure if any software examines both refcount and pincount to
determine whether it knows what all of the refcounts to a page mean,
but a human might.  If someone else calls dump_page() after we update
pincount and before we update refcount, we might make a bad decision
while debugging.  When the updates are the other way around, a spurious
extra refcount is to be expected and should not confuse anyone more than
they already are.

> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >   mm/gup.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index a9d4d724aef7..08020987dfc0 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -220,18 +220,18 @@ bool __must_check try_grab_page(struct page *page, unsigned int flags)
> >   		if (WARN_ON_ONCE(page_ref_count(page) <= 0))
> >   			return false;
> > -		if (hpage_pincount_available(page))
> > -			hpage_pincount_add(page, 1);
> > -		else
> > -			refs = GUP_PIN_COUNTING_BIAS;
> > -
> >   		/*
> >   		 * Similar to try_grab_compound_head(): even if using the
> >   		 * hpage_pincount_add/_sub() routines, be sure to
> >   		 * *also* increment the normal page refcount field at least
> >   		 * once, so that the page really is pinned.
> >   		 */
> > -		page_ref_add(page, refs);
> 
> A fine point: this hunk removes the last use of "refs", which means that
> this patch will lead to an unused variable warning. So I think it would
> be best to remove the "int refs = 1;" line in this patch, rather than
> waiting until patch #10.

Argh!  I noticed that I needed to do that, and then forgot to do it.
Thanks!

> With that change, please feel free to add:
> 
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>

Added, and git tree updated.