Hi Matthew,
Thanks for the review.
On 5/12/23 09:53, Matthew Wilcox wrote:
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.
That makes sense. I will change that.
+ 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().
I will fix that.
+ 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.
You are right.
/*
- * 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)) ?
I like that better. I will make these modifications.
Thanks,
Khalid
