On 12/7/22 6:27 PM, John Hubbard wrote:
On 12/7/22 17:42, Sidhartha Kumar wrote:
Wouldn't it be better to instead just create a new function for that
case, such as:
dissolve_large_folio()
Prior to the folio conversion, the helper function
__destroy_compound_gigantic_page() did:
set_compound_order(page, 0);
#ifdef CONFIG_64BIT
page[1].compound_nr = 0;
#endif
as part of dissolving the page. My goal for this patch was to create a
function that would encapsulate that segment of code with a single
call of folio_set_compound_order(folio, 0). set_compound_order() does
not set compound_nr to 0 when 0 is passed in to the order argument so
explicitly setting it is required. I don't think a separate
dissolve_large_folio() function for the hugetlb case is needed as
__destroy_compound_gigantic_folio() is pretty concise as it is.
Instead of "this is abusing function X()" comments, we should prefer
well-named functions that do something understandable. And you can get
that by noticing that folio_set_compound_order() collapses down to
nearly nothing in the special "order 0" case. So just inline that code
directly into __destroy_compound_gigantic_folio(), taking a moment to
fill in and consolidate the CONFIG_64BIT missing parts in mm.h.
And now you can get rid of this cruft and "abuse" comment, and instead
just end up with two simple lines of code that are crystal clear--as
they should be, in a "__destroy" function. Like this:
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 105878936485..cf227ed00945 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1754,6 +1754,7 @@ static inline void set_page_links(struct page
*page, enum zone_type zone,
#endif
}
+#ifdef CONFIG_64BIT
/**
* folio_nr_pages - The number of pages in the folio.
* @folio: The folio.
@@ -1764,13 +1765,32 @@ static inline long folio_nr_pages(struct folio
*folio)
{
if (!folio_test_large(folio))
return 1;
-#ifdef CONFIG_64BIT
return folio->_folio_nr_pages;
+}
+
+static inline void folio_set_nr_pages(struct folio *folio, long nr_pages)
+{
+ folio->_folio_nr_pages = nr_pages;
+}
#else
+/**
+ * folio_nr_pages - The number of pages in the folio.
+ * @folio: The folio.
+ *
+ * Return: A positive power of two.
+ */
+static inline long folio_nr_pages(struct folio *folio)
+{
+ if (!folio_test_large(folio))
+ return 1;
return 1L << folio->_folio_order;
-#endif
}
+static inline void folio_set_nr_pages(struct folio *folio, long nr_pages)
+{
+}
+#endif
+
/**
* folio_next - Move to the next physical folio.
* @folio: The folio we're currently operating on.
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index e3500c087893..b507a98063e6 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1344,7 +1344,8 @@ static void
__destroy_compound_gigantic_folio(struct folio *folio,
set_page_refcounted(p);
}
- folio_set_compound_order(folio, 0);
+ folio->_folio_order = 0;
+ folio_set_nr_pages(folio, 0);
__folio_clear_head(folio);
}
Yes?
This works for me, I will take this approach along with Muchun's
feedback about a wrapper function so as not to touch _folio_order
directly and send out a new version.
One question I have is if I should then get rid of
folio_set_compound_order() as hugetlb is the only compound page user
I've converted to folios so far and its use can be replaced by the
suggested folio_set_nr_pages() and folio_set_order().
Hugetlb also has one has one call to folio_set_compound_order() with a
non-zero order, should I replace this with a call to folio_set_order()
and folio_set_nr_pages() as well, or keep folio_set_compound_order() and
remove zero order support and the comment. Please let me know which
approach you would prefer.
Thanks,
Sidhartha Kumar
thanks,
