Re: [PATCH 05/17] gup: Add try_get_folio()

[Matthew Wilcox <willy@infradead.org>]

On Tue, Jan 04, 2022 at 05:25:07PM -0800, John Hubbard wrote:
> On 1/2/22 13:57, Matthew Wilcox (Oracle) wrote:
> > +	folio = page_folio(page);
> > +	if (WARN_ON_ONCE(folio_ref_count(folio) < 0))
> >   		return NULL;
> > -	if (unlikely(!page_cache_add_speculative(head, refs)))
> > +	if (unlikely(!folio_ref_try_add_rcu(folio, refs)))
> 
> I'm a little lost about the meaning and intended use of the _rcu aspects
> of folio_ref_try_add_rcu() here. For example, try_get_folio() does not
> require that callers are in an rcu read section, right? This is probably
> just a documentation question, sorry if it's obvious--I wasn't able to
> work it out on my own.

page_cache_add_speculative() always assumed that you were at least as
protected as RCU.  Quoting v5.4's pagemap.h:

 * speculatively take a reference to a page.
 * If the page is free (_refcount == 0), then _refcount is untouched, and 0
 * is returned. Otherwise, _refcount is incremented by 1 and 1 is returned.
 *
 * This function must be called inside the same rcu_read_lock() section as has
 * been used to lookup the page in the pagecache radix-tree (or page table):
 * this allows allocators to use a synchronize_rcu() to stabilize _refcount.

... so is it the addition of "rcu" in the name that's scared you?
If so, that seems like a good thing, because previously you were using
page_cache_add_speculative() without realising that RCU protection
was required.

I think there is sufficient protection; either we have interrupts disabled
in gup-fast (which prevents RCU from finishing a grace period), or we
have the mmap_sem held in gup-slow (which prevents pages from being
removed from the page tables).

But maybe I've missed something?