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

[John Hubbard <jhubbard@nvidia.com>]

On 1/2/22 13:57, Matthew Wilcox (Oracle) wrote:
> This replaces try_get_compound_head().  It includes a small optimisation
> for the race where a folio is split between being looked up from its
> tail page and the reference count being obtained.  Before, it returned
> NULL, which presumably triggered a retry under the mmap_lock, whereas
> now it will retry without the lock.  Finding a frozen page will still
> return NULL.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/gup.c | 69 +++++++++++++++++++++++++++++---------------------------
>   1 file changed, 36 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 2c51e9748a6a..58e5cfaaa676 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -29,12 +29,11 @@ struct follow_page_context {
>   	unsigned int page_mask;
>   };
>   
> -static void hpage_pincount_add(struct page *page, int refs)
> +static void folio_pincount_add(struct folio *folio, int refs)
>   {
> -	VM_BUG_ON_PAGE(!hpage_pincount_available(page), page);
> -	VM_BUG_ON_PAGE(page != compound_head(page), page);
> +	VM_BUG_ON_FOLIO(!folio_pincount_available(folio), folio);
>   
> -	atomic_add(refs, compound_pincount_ptr(page));
> +	atomic_add(refs, folio_pincount_ptr(folio));
>   }
>   
>   static void hpage_pincount_sub(struct page *page, int refs)
> @@ -63,33 +62,35 @@ static void put_page_refs(struct page *page, int refs)
>   }
>   
>   /*
> - * Return the compound head page with ref appropriately incremented,
> + * Return the folio with ref appropriately incremented,
>    * or NULL if that failed.
>    */
> -static inline struct page *try_get_compound_head(struct page *page, int refs)
> +static inline struct folio *try_get_folio(struct page *page, int refs)
>   {
> -	struct page *head = compound_head(page);
> +	struct folio *folio;
>   
> -	if (WARN_ON_ONCE(page_ref_count(head) < 0))
> +retry:

Yes, this new retry looks like a solid improvement. Retrying at this low
level makes a lot of sense, given that it is racing with a very
transient sort of behavior.


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

>   		return NULL;
>   
>   	/*
> -	 * At this point we have a stable reference to the head page; but it
> -	 * could be that between the compound_head() lookup and the refcount
> -	 * increment, the compound page was split, in which case we'd end up
> -	 * holding a reference on a page that has nothing to do with the page
> +	 * At this point we have a stable reference to the folio; but it
> +	 * could be that between calling page_folio() and the refcount
> +	 * increment, the folio was split, in which case we'd end up
> +	 * holding a reference on a folio that has nothing to do with the page
>   	 * we were given anymore.
> -	 * So now that the head page is stable, recheck that the pages still
> -	 * belong together.
> +	 * So now that the folio is stable, recheck that the page still
> +	 * belongs to this folio.
>   	 */
> -	if (unlikely(compound_head(page) != head)) {
> -		put_page_refs(head, refs);
> -		return NULL;
> +	if (unlikely(page_folio(page) != folio)) {
> +		folio_put_refs(folio, refs);
> +		goto retry;
>   	}
>   
> -	return head;
> +	return folio;
>   }
>   
>   /**
> @@ -128,8 +129,10 @@ struct page *try_grab_compound_head(struct page *page,
>   				    int refs, unsigned int flags)
>   {
>   	if (flags & FOLL_GET)
> -		return try_get_compound_head(page, refs);
> +		return &try_get_folio(page, refs)->page;


Did you want to use folio_page() here, instead?


>   	else if (flags & FOLL_PIN) {
> +		struct folio *folio;
> +
>   		/*
>   		 * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
>   		 * right zone, so fail and let the caller fall back to the slow
> @@ -143,29 +146,29 @@ struct page *try_grab_compound_head(struct page *page,
>   		 * CAUTION: Don't use compound_head() on the page before this
>   		 * point, the result won't be stable.
>   		 */
> -		page = try_get_compound_head(page, refs);
> -		if (!page)
> +		folio = try_get_folio(page, refs);
> +		if (!folio)
>   			return NULL;
>   
>   		/*
> -		 * When pinning a compound page of order > 1 (which is what
> +		 * When pinning a folio of order > 1 (which is what
>   		 * hpage_pincount_available() checks for), use an exact count to
> -		 * track it, via hpage_pincount_add/_sub().
> +		 * track it, via folio_pincount_add/_sub().
>   		 *
> -		 * However, be sure to *also* increment the normal page refcount
> -		 * field at least once, so that the page really is pinned.
> +		 * However, be sure to *also* increment the normal folio refcount
> +		 * field at least once, so that the folio really is pinned.
>   		 * That's why the refcount from the earlier
> -		 * try_get_compound_head() is left intact.
> +		 * try_get_folio() is left intact.
>   		 */
> -		if (hpage_pincount_available(page))
> -			hpage_pincount_add(page, refs);
> +		if (folio_pincount_available(folio))
> +			folio_pincount_add(folio, refs);
>   		else
> -			page_ref_add(page, refs * (GUP_PIN_COUNTING_BIAS - 1));
> +			folio_ref_add(folio,
> +					refs * (GUP_PIN_COUNTING_BIAS - 1));
>   
> -		mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_ACQUIRED,
> -				    refs);
> +		node_stat_mod_folio(folio, NR_FOLL_PIN_ACQUIRED, refs);
>   
> -		return page;
> +		return &folio->page;
>   	}
>   
>   	WARN_ON_ONCE(1);

Just minor questions above. This all looks solid though.

thanks,
-- 
John Hubbard
NVIDIA