[PATCH 0/2] Reliable testing for hugetlb

[PATCH 0/2] Reliable testing for hugetlb

[Matthew Wilcox (Oracle)]

We need to handle pages allocated by hugetlbfs differently in a number of
places.  If we have a reference to the folio, there are many methods which
will work.  If we don't, most of our previous attempts can misidentify a
page which never belonged to hugetlb as belonging to hugetlb.  It's quite
hard to hit these kinds of races, and generally the results are relatively
benign, but I don't feel good about them existing.  Obviously since we
don't have a reference to the page, the answer can be stale ("it was part
of hugetlb when we asked, but now it isn't"), but that's very different
from "This page never belonged to hugetlb, but we said it did".

We've tried various things in the past.  My most recent approach was
to use a bit in the second page's flags, reusing the "active" flag.
This is very susceptible to this problem.  Before that, we used to
check that folio_dtor (previously compound_dtor) had a specific value.
This field was also stored in the second page, shared with the LRU.
It was less susceptible to this problem, which may be why nobody noticed
it since its introduction in 2009.

We have considered various approaches to make folio_test_hugetlb() more
reliable.  We could use a word in the second page, but that occupies
space that we probably have a better use for.  We could use a word in
the third page, but then we have to test that there is a third page so
we don't run off the end of memmap.  We considered using a special value
in nr_pages_mapped, but weren't convinced we could ensure that it would
be unique if the second page was reallocated to a different purpose.

Using PageType is completely reliable.  It will never return true for a
page if that page was never in a hugetlb allocation.  It doesn't occupy
any extra space, and we have plenty of bits left in PageType.  It's also
the most efficient definition of PageHuge() we've ever had, allowing it
to be a static inline function rather than an exported function.

Matthew Wilcox (Oracle) (2):
  mm: Turn folio_test_hugetlb into a PageType
  mm: Remove a call to compound_head() from is_page_hwpoison()

 include/linux/mm.h         |  3 ++
 include/linux/page-flags.h | 73 +++++++++++++++++++-------------------
 mm/hugetlb.c               | 22 ++----------
 3 files changed, 42 insertions(+), 56 deletions(-)

-- 
2.43.0

[PATCH 1/2] mm: Turn folio_test_hugetlb into a PageType

[Matthew Wilcox (Oracle)]

The current folio_test_hugetlb() can be fooled by a concurrent folio split
into returning true for a folio which has never belonged to hugetlbfs.
This can't happen if the caller holds a refcount on it, but we have a
few places (memory-failure, compaction, procfs) which do not and should
not take a speculative reference.

Since hugetlb pages do not use individual page mapcounts (they are always
fully mapped and use the entire_mapcount field to record the number of
mappings), the PageType field is available for use with only a minor
modification to page_mapcount() to ignore the value in this field for
hugetlb pages.  Other parts of the kernel which examine page->_mapcount
directly already know about page_has_type() (eg procfs, dump_page()).

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/mm.h         |  3 ++
 include/linux/page-flags.h | 70 ++++++++++++++++++--------------------
 mm/hugetlb.c               | 22 ++----------
 3 files changed, 39 insertions(+), 56 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index f5a97dec5169..32281097f07e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1212,6 +1212,9 @@ static inline int page_mapcount(struct page *page)
 {
 	int mapcount = atomic_read(&page->_mapcount) + 1;
 
+	/* Head page repurposes mapcount; tail page leaves at -1 */
+	if (PageHeadHuge(page))
+		mapcount = 0;
 	if (unlikely(PageCompound(page)))
 		mapcount += folio_entire_mapcount(page_folio(page));
 
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 735cddc13d20..6ebb573c7195 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -190,7 +190,6 @@ enum pageflags {
 
 	/* At least one page in this folio has the hwpoison flag set */
 	PG_has_hwpoisoned = PG_error,
-	PG_hugetlb = PG_active,
 	PG_large_rmappable = PG_workingset, /* anon or file-backed */
 };
 
@@ -829,29 +828,6 @@ TESTPAGEFLAG_FALSE(LargeRmappable, large_rmappable)
 
 #define PG_head_mask ((1UL << PG_head))
 
-#ifdef CONFIG_HUGETLB_PAGE
-int PageHuge(struct page *page);
-SETPAGEFLAG(HugeTLB, hugetlb, PF_SECOND)
-CLEARPAGEFLAG(HugeTLB, hugetlb, PF_SECOND)
-
-/**
- * folio_test_hugetlb - Determine if the folio belongs to hugetlbfs
- * @folio: The folio to test.
- *
- * Context: Any context.  Caller should have a reference on the folio to
- * prevent it from being turned into a tail page.
- * Return: True for hugetlbfs folios, false for anon folios or folios
- * belonging to other filesystems.
- */
-static inline bool folio_test_hugetlb(struct folio *folio)
-{
-	return folio_test_large(folio) &&
-		test_bit(PG_hugetlb, folio_flags(folio, 1));
-}
-#else
-TESTPAGEFLAG_FALSE(Huge, hugetlb)
-#endif
-
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 /*
  * PageHuge() only returns true for hugetlbfs pages, but not for
@@ -907,18 +883,6 @@ PAGEFLAG_FALSE(HasHWPoisoned, has_hwpoisoned)
 	TESTSCFLAG_FALSE(HasHWPoisoned, has_hwpoisoned)
 #endif
 
-/*
- * Check if a page is currently marked HWPoisoned. Note that this check is
- * best effort only and inherently racy: there is no way to synchronize with
- * failing hardware.
- */
-static inline bool is_page_hwpoison(struct page *page)
-{
-	if (PageHWPoison(page))
-		return true;
-	return PageHuge(page) && PageHWPoison(compound_head(page));
-}
-
 /*
  * For pages that are never mapped to userspace (and aren't PageSlab),
  * page_type may be used.  Because it is initialised to -1, we invert the
@@ -935,6 +899,7 @@ static inline bool is_page_hwpoison(struct page *page)
 #define PG_offline	0x00000100
 #define PG_table	0x00000200
 #define PG_guard	0x00000400
+#define PG_hugetlb	0x00000800
 
 #define PageType(page, flag)						\
 	((page->page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE)
@@ -1026,6 +991,37 @@ PAGE_TYPE_OPS(Table, table, pgtable)
  */
 PAGE_TYPE_OPS(Guard, guard, guard)
 
+#ifdef CONFIG_HUGETLB_PAGE
+PAGE_TYPE_OPS(HeadHuge, hugetlb, hugetlb)
+#else
+TESTPAGEFLAG_FALSE(HeadHuge, hugetlb)
+#endif
+
+/**
+ * PageHuge - Determine if the page belongs to hugetlbfs
+ * @page: The page to test.
+ *
+ * Context: Any context.
+ * Return: True for hugetlbfs pages, false for anon pages or pages
+ * belonging to other filesystems.
+ */
+static inline bool PageHuge(struct page *page)
+{
+	return folio_test_hugetlb(page_folio(page));
+}
+
+/*
+ * Check if a page is currently marked HWPoisoned. Note that this check is
+ * best effort only and inherently racy: there is no way to synchronize with
+ * failing hardware.
+ */
+static inline bool is_page_hwpoison(struct page *page)
+{
+	if (PageHWPoison(page))
+		return true;
+	return PageHuge(page) && PageHWPoison(compound_head(page));
+}
+
 extern bool is_free_buddy_page(struct page *page);
 
 PAGEFLAG(Isolated, isolated, PF_ANY);
@@ -1092,7 +1088,7 @@ static __always_inline void __ClearPageAnonExclusive(struct page *page)
  */
 #define PAGE_FLAGS_SECOND						\
 	(0xffUL /* order */		| 1UL << PG_has_hwpoisoned |	\
-	 1UL << PG_hugetlb		| 1UL << PG_large_rmappable)
+	 1UL << PG_large_rmappable)
 
 #define PAGE_FLAGS_PRIVATE				\
 	(1UL << PG_private | 1UL << PG_private_2)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index ed1581b670d4..23b62df21971 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1623,7 +1623,7 @@ static inline void __clear_hugetlb_destructor(struct hstate *h,
 {
 	lockdep_assert_held(&hugetlb_lock);
 
-	folio_clear_hugetlb(folio);
+	__folio_clear_hugetlb(folio);
 }
 
 /*
@@ -1710,7 +1710,7 @@ static void add_hugetlb_folio(struct hstate *h, struct folio *folio,
 		h->surplus_huge_pages_node[nid]++;
 	}
 
-	folio_set_hugetlb(folio);
+	__folio_set_hugetlb(folio);
 	folio_change_private(folio, NULL);
 	/*
 	 * We have to set hugetlb_vmemmap_optimized again as above
@@ -2048,7 +2048,7 @@ static void __prep_account_new_huge_page(struct hstate *h, int nid)
 
 static void init_new_hugetlb_folio(struct hstate *h, struct folio *folio)
 {
-	folio_set_hugetlb(folio);
+	__folio_set_hugetlb(folio);
 	INIT_LIST_HEAD(&folio->lru);
 	hugetlb_set_folio_subpool(folio, NULL);
 	set_hugetlb_cgroup(folio, NULL);
@@ -2158,22 +2158,6 @@ static bool prep_compound_gigantic_folio_for_demote(struct folio *folio,
 	return __prep_compound_gigantic_folio(folio, order, true);
 }
 
-/*
- * PageHuge() only returns true for hugetlbfs pages, but not for normal or
- * transparent huge pages.  See the PageTransHuge() documentation for more
- * details.
- */
-int PageHuge(struct page *page)
-{
-	struct folio *folio;
-
-	if (!PageCompound(page))
-		return 0;
-	folio = page_folio(page);
-	return folio_test_hugetlb(folio);
-}
-EXPORT_SYMBOL_GPL(PageHuge);
-
 /*
  * Find and lock address space (mapping) in write mode.
  *
-- 
2.43.0

Re: [PATCH 1/2] mm: Turn folio_test_hugetlb into a PageType

[David Hildenbrand]

On 14.03.24 02:25, Matthew Wilcox (Oracle) wrote:
> The current folio_test_hugetlb() can be fooled by a concurrent folio split
> into returning true for a folio which has never belonged to hugetlbfs.
> This can't happen if the caller holds a refcount on it, but we have a
> few places (memory-failure, compaction, procfs) which do not and should
> not take a speculative reference.
> 
> Since hugetlb pages do not use individual page mapcounts (they are always
> fully mapped and use the entire_mapcount field to record the number of
> mappings), the PageType field is available for use with only a minor
> modification to page_mapcount() to ignore the value in this field for
> hugetlb pages.  Other parts of the kernel which examine page->_mapcount
> directly already know about page_has_type() (eg procfs, dump_page()).
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---

Should we include this hunk:

diff --git a/mm/debug.c b/mm/debug.c
index c1c1a6a484e4c..f521e17150a10 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -63,7 +63,8 @@ static void __dump_folio(struct folio *folio, struct page *page,
          * encode own info, and we must avoid calling page_folio() again.
          */
         if (!folio_test_slab(folio)) {
-               mapcount = atomic_read(&page->_mapcount) + 1;
+               if (!folio_test_hugetlb(folio))
+                       mapcount = atomic_read(&page->_mapcount) + 1;
                 if (folio_test_large(folio))
                         mapcount += folio_entire_mapcount(folio);
         }

Apart from that, LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm: Turn folio_test_hugetlb into a PageType

[Matthew Wilcox]

On Thu, Mar 14, 2024 at 11:26:17AM +0100, David Hildenbrand wrote:
> Should we include this hunk:
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index c1c1a6a484e4c..f521e17150a10 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -63,7 +63,8 @@ static void __dump_folio(struct folio *folio, struct page *page,
>          * encode own info, and we must avoid calling page_folio() again.
>          */
>         if (!folio_test_slab(folio)) {
> -               mapcount = atomic_read(&page->_mapcount) + 1;
> +               if (!folio_test_hugetlb(folio))
> +                       mapcount = atomic_read(&page->_mapcount) + 1;
>                 if (folio_test_large(folio))
>                         mapcount += folio_entire_mapcount(folio);
>         }
> 
> Apart from that, LGTM

I actually want to do a bit more here ...

+++ b/mm/debug.c
@@ -58,15 +58,10 @@ static void __dump_folio(struct folio *folio, struct page *page,
        int mapcount = 0;
        char *type = "";

-       /*
-        * page->_mapcount space in struct page is used by slab pages to
-        * encode own info, and we must avoid calling page_folio() again.
-        */
-       if (!folio_test_slab(folio)) {
+       if (!page_has_type(page)) {
                mapcount = atomic_read(&page->_mapcount) + 1;
-               if (folio_test_large(folio))
-                       mapcount += folio_entire_mapcount(folio);
-       }
+       if (folio_test_large(folio))
+               mapcount += folio_entire_mapcount(folio);

        pr_warn("page: refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
                        folio_ref_count(folio), mapcount, mapping,
@@ -99,7 +94,8 @@ static void __dump_folio(struct folio *folio, struct page *page,
         */
        pr_warn("%sflags: %pGp%s\n", type, &folio->flags,
                is_migrate_cma_folio(folio, pfn) ? " CMA" : "");
-       pr_warn("page_type: %pGt\n", &folio->page.page_type);
+       if (page_has_type(page))
+               pr_warn("type: %pGt\n", &folio->page.page_type);

        print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
                        sizeof(unsigned long), page,


and I think that deserves its own patch.

Also, I need to do:

+++ b/include/trace/events/mmflags.h
@@ -135,6 +135,7 @@ IF_HAVE_PG_ARCH_X(arch_3)
 #define DEF_PAGETYPE_NAME(_name) { PG_##_name, __stringify(_name) }

 #define __def_pagetype_names                                           \
+       DEF_PAGETYPE_NAME(hugetlb)                                      \
        DEF_PAGETYPE_NAME(offline),                                     \
        DEF_PAGETYPE_NAME(guard),                                       \
        DEF_PAGETYPE_NAME(table),                                       \

(and I think having this in the include/trace directory is a mistake;
putting it in page-flags.h will lead to it not being forgotten)

Re: [PATCH 1/2] mm: Turn folio_test_hugetlb into a PageType

[Miaohe Lin]

On 2024/3/14 22:33, Matthew Wilcox wrote:
> On Thu, Mar 14, 2024 at 11:26:17AM +0100, David Hildenbrand wrote:
>> Should we include this hunk:
>>
>> diff --git a/mm/debug.c b/mm/debug.c
>> index c1c1a6a484e4c..f521e17150a10 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -63,7 +63,8 @@ static void __dump_folio(struct folio *folio, struct page *page,
>>          * encode own info, and we must avoid calling page_folio() again.
>>          */
>>         if (!folio_test_slab(folio)) {
>> -               mapcount = atomic_read(&page->_mapcount) + 1;
>> +               if (!folio_test_hugetlb(folio))
>> +                       mapcount = atomic_read(&page->_mapcount) + 1;
>>                 if (folio_test_large(folio))
>>                         mapcount += folio_entire_mapcount(folio);
>>         }
>>
>> Apart from that, LGTM
> 
> I actually want to do a bit more here ...
> 
> +++ b/mm/debug.c
> @@ -58,15 +58,10 @@ static void __dump_folio(struct folio *folio, struct page *page,
>         int mapcount = 0;
>         char *type = "";
> 
> -       /*
> -        * page->_mapcount space in struct page is used by slab pages to
> -        * encode own info, and we must avoid calling page_folio() again.
> -        */
> -       if (!folio_test_slab(folio)) {
> +       if (!page_has_type(page)) {
>                 mapcount = atomic_read(&page->_mapcount) + 1;
> -               if (folio_test_large(folio))
> -                       mapcount += folio_entire_mapcount(folio);
> -       }
> +       if (folio_test_large(folio))
> +               mapcount += folio_entire_mapcount(folio);

Why folio_test_large is not within the "if (!page_has_type(page))" block? I think a slab page could also be large folio.
Or am I miss something?

Thanks.

Re: [PATCH 1/2] mm: Turn folio_test_hugetlb into a PageType

[Matthew Wilcox]

On Fri, Mar 15, 2024 at 02:19:10PM +0800, Miaohe Lin wrote:
> On 2024/3/14 22:33, Matthew Wilcox wrote:
> > I actually want to do a bit more here ...
> > 
> > +++ b/mm/debug.c
> > @@ -58,15 +58,10 @@ static void __dump_folio(struct folio *folio, struct page *page,
> >         int mapcount = 0;
> >         char *type = "";
> > 
> > -       /*
> > -        * page->_mapcount space in struct page is used by slab pages to
> > -        * encode own info, and we must avoid calling page_folio() again.
> > -        */
> > -       if (!folio_test_slab(folio)) {
> > +       if (!page_has_type(page)) {
> >                 mapcount = atomic_read(&page->_mapcount) + 1;
> > -               if (folio_test_large(folio))
> > -                       mapcount += folio_entire_mapcount(folio);
> > -       }
> > +       if (folio_test_large(folio))
> > +               mapcount += folio_entire_mapcount(folio);
> 
> Why folio_test_large is not within the "if (!page_has_type(page))" block? I think a slab page could also be large folio.
> Or am I miss something?

Slab pages don't use the first tail page so folio_entire_mapcount will
be 0 for them.  And we don't want the folio_test_large() to be gated by
page_has_type() because hugetlb pages (after this patch) will be in the
page_has_type() category.  So for hugetlb pages, we want to not read
page->mapcount (leaving mapcount at zero), but we do want to read the
entire_mapcount.

Re: [PATCH 1/2] mm: Turn folio_test_hugetlb into a PageType

[Miaohe Lin]

On 2024/3/15 20:32, Matthew Wilcox wrote:
> On Fri, Mar 15, 2024 at 02:19:10PM +0800, Miaohe Lin wrote:
>> On 2024/3/14 22:33, Matthew Wilcox wrote:
>>> I actually want to do a bit more here ...
>>>
>>> +++ b/mm/debug.c
>>> @@ -58,15 +58,10 @@ static void __dump_folio(struct folio *folio, struct page *page,
>>>         int mapcount = 0;
>>>         char *type = "";
>>>
>>> -       /*
>>> -        * page->_mapcount space in struct page is used by slab pages to
>>> -        * encode own info, and we must avoid calling page_folio() again.
>>> -        */
>>> -       if (!folio_test_slab(folio)) {
>>> +       if (!page_has_type(page)) {
>>>                 mapcount = atomic_read(&page->_mapcount) + 1;
>>> -               if (folio_test_large(folio))
>>> -                       mapcount += folio_entire_mapcount(folio);
>>> -       }
>>> +       if (folio_test_large(folio))
>>> +               mapcount += folio_entire_mapcount(folio);
>>
>> Why folio_test_large is not within the "if (!page_has_type(page))" block? I think a slab page could also be large folio.
>> Or am I miss something?
> 
> Slab pages don't use the first tail page so folio_entire_mapcount will
> be 0 for them.  And we don't want the folio_test_large() to be gated by
> page_has_type() because hugetlb pages (after this patch) will be in the
> page_has_type() category.  So for hugetlb pages, we want to not read
> page->mapcount (leaving mapcount at zero), but we do want to read the
> entire_mapcount.

I see. Many thanks for your explanation. This might worth a comment to avoid future confusion.
Thanks.

[PATCH 2/2] mm: Remove a call to compound_head() from is_page_hwpoison()

[Matthew Wilcox (Oracle)]

We can call it only once instead of twice.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/page-flags.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 6ebb573c7195..f1d65a72b378 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -1017,9 +1017,12 @@ static inline bool PageHuge(struct page *page)
  */
 static inline bool is_page_hwpoison(struct page *page)
 {
+	struct folio *folio;
+
 	if (PageHWPoison(page))
 		return true;
-	return PageHuge(page) && PageHWPoison(compound_head(page));
+	folio = page_folio(page);
+	return folio_test_hugetlb(folio) && PageHWPoison(&folio->page);
 }
 
 extern bool is_free_buddy_page(struct page *page);
-- 
2.43.0

Re: [PATCH 2/2] mm: Remove a call to compound_head() from is_page_hwpoison()

[David Hildenbrand]

On 14.03.24 02:25, Matthew Wilcox (Oracle) wrote:
> We can call it only once instead of twice.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   include/linux/page-flags.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6ebb573c7195..f1d65a72b378 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -1017,9 +1017,12 @@ static inline bool PageHuge(struct page *page)
>    */
>   static inline bool is_page_hwpoison(struct page *page)
>   {
> +	struct folio *folio;
> +
>   	if (PageHWPoison(page))
>   		return true;
> -	return PageHuge(page) && PageHWPoison(compound_head(page));
> +	folio = page_folio(page);
> +	return folio_test_hugetlb(folio) && PageHWPoison(&folio->page);
>   }
>   
>   extern bool is_free_buddy_page(struct page *page);

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] mm: Remove a call to compound_head() from is_page_hwpoison()

[Miaohe Lin]

On 2024/3/14 9:25, Matthew Wilcox (Oracle) wrote:
> We can call it only once instead of twice.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Thanks.

> ---
>  include/linux/page-flags.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6ebb573c7195..f1d65a72b378 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -1017,9 +1017,12 @@ static inline bool PageHuge(struct page *page)
>   */
>  static inline bool is_page_hwpoison(struct page *page)
>  {
> +	struct folio *folio;
> +
>  	if (PageHWPoison(page))
>  		return true;
> -	return PageHuge(page) && PageHWPoison(compound_head(page));
> +	folio = page_folio(page);
> +	return folio_test_hugetlb(folio) && PageHWPoison(&folio->page);
>  }
>  
>  extern bool is_free_buddy_page(struct page *page);
>