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