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

[John Hubbard <jhubbard@nvidia.com>]

On 1/7/22 17:37, Matthew Wilcox wrote:
> 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.

Thanks for the v5.4 documentation reference. I did read and understand that a
while back, but it didn't show up at all when I was looking through the code
during this review, so I missed a chance to re-read it. I'd forgotten about the
RCU qualifier there.

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

Clearly, I'm not the right person to answer that question, at this point. :)

FWIW, I went back and looked again, and it seems like everything is covered.

On the documentation front, I do miss the v5.4 helpful comment block above
__page_cache_add_speculative(), it would be nice to have an updated version
of that revived, but of course that's a separate point from this patchset.

thanks,
-- 
John Hubbard
NVIDIA