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?
