Re: [PATCH] mm, compaction: Skip all pinned pages during scan

[Matthew Wilcox <willy@infradead.org>]

On Thu, May 11, 2023 at 10:55:16AM -0600, Khalid Aziz wrote:
> +/*
> + * Check if this base page should be skipped from isolation because
> + * it is pinned. This function is called for regular pages only, and not
> + * for THP or hugetlbfs pages. This code is inspired by similar code
> + * in migrate_vma_check_page(), can_split_folio() and
> + * folio_migrate_mapping()
> + */
> +static inline bool is_pinned_page(struct page *page)

... yet another reminder this file hasn't been converted to folios :-(
This part is particularly hard because we don't have a refcount on the
page yet, so it may be allocated or freed while we're looking at it
which means we can't use folios _here_ because the Tail flag may get
set which would cause the folio code to drop BUGs all over us.

> +{
> +	unsigned long extra_refs;
> +
> +	/* anonymous page can have extra ref from page cache */
> +	if (page_mapping(page))

We already did the work of calling page_mapping() in the caller.
Probably best to pass it in here.

> +		extra_refs = 1 + page_has_private(page);

page_has_private() is wrong.  That's for determining if we need to call
the release function.  Filesystems don't increment the refcount when
they set PG_private_2.  This should just be PagePrivate().

> +	else
> +		extra_refs = PageSwapCache(page) ? 1 : 0;
> +
> +	/*
> +	 * This is an admittedly racy check but good enough to determine
> +	 * if a page should be isolated
> +	 */
> +	if ((page_count(page) - extra_refs) > page_mapcount(page))

page_count() includes a hidden call to compound_head(); you probably
meant page_ref_count() here.

>  		/*
> -		 * Migration will fail if an anonymous page is pinned in memory,
> -		 * so avoid taking lru_lock and isolating it unnecessarily in an
> -		 * admittedly racy check.
> +		 * Migration will fail if a page is pinned in memory,
> +		 * so avoid taking lru_lock and isolating it unnecessarily
>  		 */
>  		mapping = page_mapping(page);
> -		if (!mapping && (page_count(page) - 1) > total_mapcount(page))
> +		if (is_pinned_page(page))

"pinned" now has two meanings when applied to pages, alas.  Better to
say "If there are extra references to this page beyond those from the
page/swap cache and page tables".

So it's probably also unwise to call it is_pinned_page().  Maybe

		if (page_extra_refcounts(page)) ?