Re: [PATCH RFC v3] mm: Proper document tail pages fields for folio

From: David Hildenbrand
Date: Wed Aug 16 2023 - 09:35:08 EST

On 15.08.23 23:25, Peter Xu wrote:
Tail page struct reuse is over-comlicated. Not only because we have

It is complicated, agreed.

With the ->private for THP_SWAP gone, we would have to document less.
Stating that 4*4byte / 4*8 byte are available after flags+head would
be sufficient and I'd even drop the table.


implicit uses of tail page fields (mapcounts, or private for thp swap
support, etc., that we may still use in the page structs,

Instead of documenting that thp swap should no longer touch the private
field of tail pages, maybe we can indeed fix that quite easily.

My simple tests passed so far. If there isn't something obvious missing,
I can do more testing and send this as an official patch.


From ec0f8b0dd8fb81c316b6a4c5fc9ae7563e625404 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@xxxxxxxxxx>
Date: Wed, 16 Aug 2023 13:14:45 +0200
Subject: [PATCH] mm/swap: stop using page->private on tail pages for THP_SWAP

Let's stop using page->private on tail pages, making it possible to
just unconditionally reuse that field in the tail pages of large folios.

Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
---
arch/arm64/mm/mteswap.c | 5 +++--
include/linux/swap.h | 9 +++++++++
mm/huge_memory.c | 15 ++++++---------
mm/memory.c | 2 +-
mm/rmap.c | 2 +-
mm/swap_state.c | 4 ++--
mm/swapfile.c | 4 ++--
7 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
index cd508ba80ab1..a31833e3ddc5 100644
--- a/arch/arm64/mm/mteswap.c
+++ b/arch/arm64/mm/mteswap.c
@@ -33,8 +33,9 @@ int mte_save_tags(struct page *page)
mte_save_page_tags(page_address(page), tag_storage);
- /* page_private contains the swap entry.val set in do_swap_page */
- ret = xa_store(&mte_pages, page_private(page), tag_storage, GFP_KERNEL);
+ /* lookup the swap entry.val from the page */
+ ret = xa_store(&mte_pages, page_swap_entry(page).val, tag_storage,
+ GFP_KERNEL);
if (WARN(xa_is_err(ret), "Failed to store MTE tags")) {
mte_free_tag_storage(tag_storage);
return xa_err(ret);
diff --git a/include/linux/swap.h b/include/linux/swap.h
index bb5adc604144..84fe0e94f5cd 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -339,6 +339,15 @@ static inline swp_entry_t folio_swap_entry(struct folio *folio)
return entry;
}
+static inline swp_entry_t page_swap_entry(struct page *page)
+{
+ struct folio *folio = page_folio(page);
+ swp_entry_t entry = folio_swap_entry(folio);
+
+ entry.val += page - &folio->page;
+ return entry;
+}
+
static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
{
folio->private = (void *)entry.val;
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0b709d2c46c6..f7e04cbcb063 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2451,18 +2451,15 @@ static void __split_huge_page_tail(struct page *head, int tail,
page_tail->index = head->index + tail;
/*
- * page->private should not be set in tail pages with the exception
- * of swap cache pages that store the swp_entry_t in tail pages.
- * Fix up and warn once if private is unexpectedly set.
- *
- * What of 32-bit systems, on which folio->_pincount overlays
- * head[1].private? No problem: THP_SWAP is not enabled on 32-bit, and
- * pincount must be 0 for folio_ref_freeze() to have succeeded.
+ * page->private should not be set in tail pages. Fix up and warn once
+ * if private is unexpectedly set.
*/
- if (!folio_test_swapcache(page_folio(head))) {
- VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, page_tail);
+ if (unlikely(page_tail->private)) {
+ VM_WARN_ON_ONCE_PAGE(true, page_tail);
page_tail->private = 0;
}
+ if (PageSwapCache(head))
+ set_page_private(page_tail, (unsigned long)head->private + tail);
/* Page flags must be visible before we make the page non-compound. */
smp_wmb();
diff --git a/mm/memory.c b/mm/memory.c
index d003076b218d..ff13242c1589 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3882,7 +3882,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
* changed.
*/
if (unlikely(!folio_test_swapcache(folio) ||
- page_private(page) != entry.val))
+ page_swap_entry(page).val != entry.val))
goto out_page;
/*
diff --git a/mm/rmap.c b/mm/rmap.c
index 1f04debdc87a..ec7f8e6c9e48 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1647,7 +1647,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
*/
dec_mm_counter(mm, mm_counter(&folio->page));
} else if (folio_test_anon(folio)) {
- swp_entry_t entry = { .val = page_private(subpage) };
+ swp_entry_t entry = page_swap_entry(subpage);
pte_t swp_pte;
/*
* Store the swap location in the pte.
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 01f15139b7d9..450819934e34 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -100,6 +100,7 @@ int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
folio_ref_add(folio, nr);
folio_set_swapcache(folio);
+ folio_set_swap_entry(folio, entry);
do {
xas_lock_irq(&xas);
@@ -113,7 +114,6 @@ int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
if (shadowp)
*shadowp = old;
}
- set_page_private(folio_page(folio, i), entry.val + i);
xas_store(&xas, folio);
xas_next(&xas);
}
@@ -154,9 +154,9 @@ void __delete_from_swap_cache(struct folio *folio,
for (i = 0; i < nr; i++) {
void *entry = xas_store(&xas, shadow);
VM_BUG_ON_PAGE(entry != folio, entry);
- set_page_private(folio_page(folio, i), 0);
xas_next(&xas);
}
+ folio->private = 0;
folio_clear_swapcache(folio);
address_space->nrpages -= nr;
__node_stat_mod_folio(folio, NR_FILE_PAGES, -nr);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index d46933adf789..bd9d904671b9 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3369,7 +3369,7 @@ struct swap_info_struct *swp_swap_info(swp_entry_t entry)
struct swap_info_struct *page_swap_info(struct page *page)
{
- swp_entry_t entry = { .val = page_private(page) };
+ swp_entry_t entry = page_swap_entry(page);
return swp_swap_info(entry);
}
@@ -3384,7 +3384,7 @@ EXPORT_SYMBOL_GPL(swapcache_mapping);
pgoff_t __page_file_index(struct page *page)
{
- swp_entry_t swap = { .val = page_private(page) };
+ swp_entry_t swap = page_swap_entry(page);
return swp_offset(swap);
}
EXPORT_SYMBOL_GPL(__page_file_index);
--
2.41.0


--
Cheers,

David / dhildenb