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
