On 10.04.25 23:54, Matthew Wilcox wrote:
On Thu, Apr 10, 2025 at 01:46:06PM -0700, Dan Williams wrote:
Matthew Wilcox wrote:
On Thu, Apr 10, 2025 at 01:15:07PM -0700, Dan Williams wrote:
For consistency and clarity what about this incremental change, to make
the __split_folio_to_order() path reuse folio_reset_order(), and use
typical bitfield helpers for manipulating _flags_1?
I dislike this intensely. It obfuscates rather than providing clarity.
I'm used to pushing folks to use bitfield.h in driver land, but will not
push it further here.
I think it can make sense in places. Just not here.
What about this hunk?
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2a47682d1ab7..301ca9459122 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3404,7 +3404,7 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
if (new_order)
folio_set_order(folio, new_order);
else
- ClearPageCompound(&folio->page);
+ folio_reset_order(folio);
}
I think that's wrong. We're splitting this folio into order-0 folios,
but folio_reset_order() is going to modify folio->_flags_1 which is in
the next page.
Right, clearing in this context might only make sense at the very start.
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2a47682d1ab77..4cd8b394b83a5 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3312,6 +3312,8 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
long nr_pages = 1 << old_order;
long i;
+ folio_reset_order(folio);
+
/*
* Skip the first new_nr_pages, since the new folio from them have all
* the flags from the original folio.
While it looks cleaner, it's in practice not required here, because
1) We handle _nr_pages overlay by setting new_folio->memcg_data
new_folio->memcg_data = folio->memcg_data;
2) We handle the order by setting new_folio->flags
new_folio->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
That should clear all flags (excluding hwpoison), including the order.
It's worth noting that in free_pages_prepare(), we handle both using
if (compound) {
page[1].flags &= ~PAGE_FLAGS_SECOND;
#ifdef NR_PAGES_IN_LARGE_FOLIO
folio->_nr_pages = 0;
#endif
--
Cheers,
David / dhildenb
