mm: remove nth_page()

Let's check that no hstate that corresponds to an unreasonable folio size is registered by an architecture. If we were to succeed registering, we could later try allocating an unsupported gigantic folio size.

Further, let's add a BUILD_BUG_ON() for checking that HUGETLB_PAGE_ORDER is sane at build time. As HUGETLB_PAGE_ORDER is dynamic on powerpc, we have to use a BUILD_BUG_ON_INVALID() to make it compile.

No existing kernel configuration should be able to trigger this check:
either SPARSEMEM without SPARSEMEM_VMEMMAP cannot be configured or gigantic folios will not exceed a memory section (the case on sparse).

Reviewed-by Zi Yan <[email protected]>

Reviewed-by Lorenzo Stoakes <[email protected]>

Reviewed-by Liam R. Howlett <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/hugetlb.c | 2 ++
  1 file changed, 2 insertions(+)

mm/hugetlb.c +2 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)
 
  	BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE <
  			__NR_HPAGEFLAGS);
+	BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER);
 
  	if (!hugepages_supported()) {
  		if (hugetlb_max_hstate || default_hstate_max_huge_pages)

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int order)
  	}
  	BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
  	BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
+	WARN_ON(order > MAX_FOLIO_ORDER);
  	h = &hstates[hugetlb_max_hstate++];
  	__mutex_init(&h->resize_lock, "resize mutex", &h->resize_key);
  	h->order = order;

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 12 lines

Let's check that no hstate that corresponds to an unreasonable folio size is registered by an architecture. If we were to succeed registering, we could later try allocating an unsupported gigantic folio size.

Further, let's add a BUILD_BUG_ON() for checking that HUGETLB_PAGE_ORDER is sane at build time. As HUGETLB_PAGE_ORDER is dynamic on powerpc, we have to use a BUILD_BUG_ON_INVALID() to make it compile.

No existing kernel configuration should be able to trigger this check:
either SPARSEMEM without SPARSEMEM_VMEMMAP cannot be configured or gigantic folios will not exceed a memory section (the case on sparse).

Quoted 4 lines

Reviewed-by Zi Yan <[email protected]>

Reviewed-by Lorenzo Stoakes <[email protected]>

Reviewed-by Liam R. Howlett <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

I get following warning on powerpc with linus tree, bisected to commit 7b4f21f5e038 ("mm/hugetlb: check for unreasonable folio sizes when registering hstate")

Quoted 14 lines

---
   mm/hugetlb.c | 2 ++
   1 file changed, 2 insertions(+)

mm/hugetlb.c +1 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)
 
   	BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE <
   			__NR_HPAGEFLAGS);
+	BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER);
 

mm/hugetlb.c void __init hugetlb_add_hstate(unsigned int order) · line 4747

Quoted 10 lines

mm/hugetlb.c +1 -0

   	if (!hugepages_supported()) {
   		if (hugetlb_max_hstate || default_hstate_max_huge_pages)

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int order)
   	}
   	BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
   	BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
+	WARN_ON(order > MAX_FOLIO_ORDER);
   	h = &hstates[hugetlb_max_hstate++];
   	__mutex_init(&h->resize_lock, "resize mutex", &h->resize_key);
   	h->order = order;

Quoted 20 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 16 lines

Let's check that no hstate that corresponds to an unreasonable folio size is registered by an architecture. If we were to succeed registering, we could later try allocating an unsupported gigantic folio size.

Further, let's add a BUILD_BUG_ON() for checking that HUGETLB_PAGE_ORDER is sane at build time. As HUGETLB_PAGE_ORDER is dynamic on powerpc, we have to use a BUILD_BUG_ON_INVALID() to make it compile.

No existing kernel configuration should be able to trigger this check:
either SPARSEMEM without SPARSEMEM_VMEMMAP cannot be configured or gigantic folios will not exceed a memory section (the case on sparse).

Reviewed-by Zi Yan <[email protected]>

Reviewed-by Lorenzo Stoakes <[email protected]>

Reviewed-by Liam R. Howlett <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

Quoted 3 lines

I get following warning on powerpc with linus tree, bisected to commit 7b4f21f5e038 ("mm/hugetlb: check for unreasonable folio sizes when registering hstate")

Do you have the kernel config around? Is it 32bit?

That would be helpful.

Quoted 15 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 12 lines

mm/hugetlb.c +0 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)

       BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE < __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER); if (!hugepages_supported()) {
           if (hugetlb_max_hstate || default_hstate_max_huge_pages)

mm/hugetlb.c +0 -0

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int order)
        }

Quoted 6 lines

mm/hugetlb.c void __init hugetlb_add_hstate(unsigned int order) · line 4747

Quoted 6 lines

mm/hugetlb.c +1 -0

        BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
        BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
+    WARN_ON(order > MAX_FOLIO_ORDER);
        h = &hstates[hugetlb_max_hstate++];
        __mutex_init(&h->resize_lock, "resize mutex", &h->resize_key);
        h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that (and if we have to support that).

Le 09/10/2025 à 09:22, David Hildenbrand a écrit :

Quoted 18 lines · Christophe Leroy

Quoted 17 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 14 lines

mm/hugetlb.c +1 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)
        BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE <
                __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER);
        if (!hugepages_supported()) {
            if (hugetlb_max_hstate || default_hstate_max_huge_pages)

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int order)
        }
        BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
        BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));

Quoted 9 lines

Quoted 4 lines

mm/hugetlb.c void __init hugetlb_add_hstate(unsigned int order) · line 4747

Quoted 4 lines

mm/hugetlb.c +1 -0

+    WARN_ON(order > MAX_FOLIO_ORDER);
        h = &hstates[hugetlb_max_hstate++];
        __mutex_init(&h->resize_lock, "resize mutex", &h->resize_key);
        h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that (and if we have to support that).

MAX_FOLIO_ORDER is defined as:

#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
#define MAX_FOLIO_ORDER PUD_ORDER
#else
#define MAX_FOLIO_ORDER MAX_PAGE_ORDER
#endif

MAX_PAGE_ORDER is the limit for dynamic creation of hugepages via /sys/kernel/mm/hugepages/ but bigger pages can be created at boottime with kernel boot parameters without CONFIG_ARCH_HAS_GIGANTIC_PAGE:

hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1

Gives:

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 64.0 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 64.0 MiB page
HugeTLB: registered 256 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 256 MiB page
HugeTLB: registered 4.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 4.00 MiB page
HugeTLB: registered 16.0 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 16.0 MiB page

Quoted 48 lines

Le 09/10/2025 à 09:22, David Hildenbrand a écrit :

Quoted 19 lines · Christophe Leroy

Quoted 18 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 15 lines

mm/hugetlb.c +0 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)

        BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE < __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER); if (!hugepages_supported()) {
            if (hugetlb_max_hstate || default_hstate_max_huge_pages)

mm/hugetlb.c +0 -0

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int 
 order)
         }
         BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
         BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

mm/hugetlb.c +1 -0

+    WARN_ON(order > MAX_FOLIO_ORDER);
         h = &hstates[hugetlb_max_hstate++];
         __mutex_init(&h->resize_lock, "resize mutex", &h->resize_key);

        h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that (and if we have to support that).

MAX_FOLIO_ORDER is defined as:

#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
#define MAX_FOLIO_ORDER        PUD_ORDER
#else
#define MAX_FOLIO_ORDER        MAX_PAGE_ORDER
#endif

MAX_PAGE_ORDER is the limit for dynamic creation of hugepages via /sys/kernel/mm/hugepages/ but bigger pages can be created at boottime with kernel boot parameters without CONFIG_ARCH_HAS_GIGANTIC_PAGE:

    hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1

Gives:

Quoted 10 lines

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 64.0 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 64.0 MiB page
HugeTLB: registered 256 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 256 MiB page
HugeTLB: registered 4.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 4.00 MiB page
HugeTLB: registered 16.0 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 16.0 MiB page

I think it's a violation of CONFIG_ARCH_HAS_GIGANTIC_PAGE. The existing folio_dump() code would not handle it correctly as well.

See how snapshot_page() uses MAX_FOLIO_NR_PAGES.

Le 09/10/2025 à 10:14, David Hildenbrand a écrit :

Quoted 61 lines · Christophe Leroy

Quoted 49 lines

Le 09/10/2025 à 09:22, David Hildenbrand a écrit :

Quoted 20 lines · Christophe Leroy

Quoted 19 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 16 lines

mm/hugetlb.c +0 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)
         BUILD_BUG_ON(sizeof_field(struct page, private) *

BITS_PER_BYTE <
                __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER);
        if (!hugepages_supported()) {
            if (hugetlb_max_hstate || default_hstate_max_huge_pages)

mm/hugetlb.c +0 -0

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int
 order)
         }
         BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
         BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

mm/hugetlb.c +1 -0

+    WARN_ON(order > MAX_FOLIO_ORDER);
         h = &hstates[hugetlb_max_hstate++];
         __mutex_init(&h->resize_lock, "resize mutex", &h->resize_key);

        h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that (and if we have to support that).

MAX_FOLIO_ORDER is defined as:

#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
#define MAX_FOLIO_ORDER        PUD_ORDER
#else
#define MAX_FOLIO_ORDER        MAX_PAGE_ORDER
#endif

MAX_PAGE_ORDER is the limit for dynamic creation of hugepages via /sys/kernel/mm/hugepages/ but bigger pages can be created at boottime with kernel boot parameters without CONFIG_ARCH_HAS_GIGANTIC_PAGE:

    hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1

Gives:

Quoted 10 lines

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 64.0 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 64.0 MiB page
HugeTLB: registered 256 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 256 MiB page
HugeTLB: registered 4.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 4.00 MiB page
HugeTLB: registered 16.0 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 16.0 MiB page

Quoted 2 lines

I think it's a violation of CONFIG_ARCH_HAS_GIGANTIC_PAGE. The existing folio_dump() code would not handle it correctly as well.

I'm trying to dig into history and when looking at commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") I understand that CONFIG_ARCH_HAS_GIGANTIC_PAGE is needed to be able to allocate gigantic pages at runtime. It is not needed to reserve gigantic pages at boottime.

What am I missing ?

Quoted 74 lines

Le 09/10/2025 à 10:14, David Hildenbrand a écrit :

Quoted 62 lines · Christophe Leroy

Quoted 50 lines

Le 09/10/2025 à 09:22, David Hildenbrand a écrit :

Quoted 23 lines · Christophe Leroy

Quoted 22 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 19 lines

mm/hugetlb.c +0 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)

         BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE <
                 __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER); if (!hugepages_supported()) {
             if (hugetlb_max_hstate || default_hstate_max_huge_pages)

mm/hugetlb.c +1 -0

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int
 order)
          }
          BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
          BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
+    WARN_ON(order > MAX_FOLIO_ORDER);
          h = &hstates[hugetlb_max_hstate++];

         __mutex_init(&h->resize_lock, "resize mutex", &h-

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

resize_key);

         h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that (and if we have to support that).

MAX_FOLIO_ORDER is defined as:

#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
#define MAX_FOLIO_ORDER        PUD_ORDER
#else
#define MAX_FOLIO_ORDER        MAX_PAGE_ORDER
#endif

MAX_PAGE_ORDER is the limit for dynamic creation of hugepages via /sys/kernel/mm/hugepages/ but bigger pages can be created at boottime with kernel boot parameters without CONFIG_ARCH_HAS_GIGANTIC_PAGE:

     hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1

Gives:

Quoted 10 lines

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 64.0 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 64.0 MiB page
HugeTLB: registered 256 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 256 MiB page
HugeTLB: registered 4.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 4.00 MiB page
HugeTLB: registered 16.0 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 16.0 MiB page

Quoted 2 lines

I think it's a violation of CONFIG_ARCH_HAS_GIGANTIC_PAGE. The existing folio_dump() code would not handle it correctly as well.

I'm trying to dig into history and when looking at commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") I understand that CONFIG_ARCH_HAS_GIGANTIC_PAGE is needed to be able to allocate gigantic pages at runtime. It is not needed to reserve gigantic pages at boottime.

Quoted 1 line

What am I missing ?

That CONFIG_ARCH_HAS_GIGANTIC_PAGE has nothing runtime-specific in its name.

Can't we just select CONFIG_ARCH_HAS_GIGANTIC_PAGE for the relevant hugetlb config that allows for *gigantic pages*.

Le 09/10/2025 à 11:20, David Hildenbrand a écrit :

Quoted 77 lines · Christophe Leroy

Quoted 76 lines

Le 09/10/2025 à 10:14, David Hildenbrand a écrit :

Quoted 64 lines · Christophe Leroy

Quoted 52 lines

Le 09/10/2025 à 09:22, David Hildenbrand a écrit :

Quoted 24 lines · Christophe Leroy

Quoted 23 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 20 lines

mm/hugetlb.c +0 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)
          BUILD_BUG_ON(sizeof_field(struct page, private) *

BITS_PER_BYTE <
                 __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER);
         if (!hugepages_supported()) {
             if (hugetlb_max_hstate ||
default_hstate_max_huge_pages)

mm/hugetlb.c +1 -0

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int
 order)
          }
          BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
          BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
+    WARN_ON(order > MAX_FOLIO_ORDER);
          h = &hstates[hugetlb_max_hstate++];
          __mutex_init(&h->resize_lock, "resize mutex", &h-

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

resize_key);

         h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that
(and if we have to support that).

MAX_FOLIO_ORDER is defined as:

#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
#define MAX_FOLIO_ORDER        PUD_ORDER
#else
#define MAX_FOLIO_ORDER        MAX_PAGE_ORDER
#endif

MAX_PAGE_ORDER is the limit for dynamic creation of hugepages via /sys/kernel/mm/hugepages/ but bigger pages can be created at boottime with kernel boot parameters without CONFIG_ARCH_HAS_GIGANTIC_PAGE:

     hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1

Gives:

Quoted 10 lines

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 64.0 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 64.0 MiB page
HugeTLB: registered 256 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 256 MiB page
HugeTLB: registered 4.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 4.00 MiB page
HugeTLB: registered 16.0 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 16.0 MiB page

Quoted 2 lines

I think it's a violation of CONFIG_ARCH_HAS_GIGANTIC_PAGE. The existing folio_dump() code would not handle it correctly as well.

I'm trying to dig into history and when looking at commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") I understand that CONFIG_ARCH_HAS_GIGANTIC_PAGE is needed to be able to allocate gigantic pages at runtime. It is not needed to reserve gigantic pages at boottime.

Quoted 4 lines

Quoted 1 line

What am I missing ?

That CONFIG_ARCH_HAS_GIGANTIC_PAGE has nothing runtime-specific in its name.

In its name for sure, but the commit I mention says:

On systems without CONTIG_ALLOC activated but that support gigantic
pages,
boottime reserved gigantic pages can not be freed at all. This patch simply enables the possibility to hand back those pages to memory allocator.

And one of the hunks is:

arch/arm64/Kconfig +1 -1

index 7f7fbd8bd9d5b..7a1aa53d188d3 100644
@@ -19,7 +19,7 @@ config ARM64
          select ARCH_HAS_FAST_MULTIPLIER
          select ARCH_HAS_FORTIFY_SOURCE
          select ARCH_HAS_GCOV_PROFILE_ALL
-       select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
+       select ARCH_HAS_GIGANTIC_PAGE
          select ARCH_HAS_KCOV
          select ARCH_HAS_KEEPINITRD
          select ARCH_HAS_MEMBARRIER_SYNC_CORE

So I understand from the commit message that it was possible at that time to have gigantic pages without ARCH_HAS_GIGANTIC_PAGE as long as you didn't have to be able to free them during runtime.

Quoted 5 lines

Can't we just select CONFIG_ARCH_HAS_GIGANTIC_PAGE for the relevant hugetlb config that allows for *gigantic pages*.

We probably can, but I'd really like to understand history and how we ended up in the situation we are now.
Because blind fixes often lead to more problems.

If I follow things correctly I see a helper gigantic_page_supported() added by commit 944d9fec8d7a ("hugetlb: add support for gigantic page allocation at runtime").

And then commit 461a7184320a ("mm/hugetlb: introduce ARCH_HAS_GIGANTIC_PAGE") is added to wrap gigantic_page_supported()

Then commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") changed gigantic_page_supported() to gigantic_page_runtime_supported()

So where are we now ?

Quoted 85 lines

Le 09/10/2025 à 11:20, David Hildenbrand a écrit :

Quoted 77 lines · Christophe Leroy

Quoted 76 lines

Le 09/10/2025 à 10:14, David Hildenbrand a écrit :

Quoted 64 lines · Christophe Leroy

Quoted 52 lines

Le 09/10/2025 à 09:22, David Hildenbrand a écrit :

Quoted 24 lines · Christophe Leroy

Quoted 23 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 20 lines

mm/hugetlb.c +0 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)

          BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE <
                  __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER); if (!hugepages_supported()) {
              if (hugetlb_max_hstate || default_hstate_max_huge_pages)

mm/hugetlb.c +1 -0

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int
 order)
           }
           BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
           BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
+    WARN_ON(order > MAX_FOLIO_ORDER);
           h = &hstates[hugetlb_max_hstate++];

          __mutex_init(&h->resize_lock, "resize mutex", &h-

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

resize_key);

          h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that
(and if we have to support that).

MAX_FOLIO_ORDER is defined as:

#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
#define MAX_FOLIO_ORDER        PUD_ORDER
#else
#define MAX_FOLIO_ORDER        MAX_PAGE_ORDER
#endif

MAX_PAGE_ORDER is the limit for dynamic creation of hugepages via /sys/kernel/mm/hugepages/ but bigger pages can be created at boottime with kernel boot parameters without CONFIG_ARCH_HAS_GIGANTIC_PAGE:

      hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1

Gives:

Quoted 10 lines

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 64.0 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 64.0 MiB page
HugeTLB: registered 256 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 256 MiB page
HugeTLB: registered 4.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 4.00 MiB page
HugeTLB: registered 16.0 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 16.0 MiB page

Quoted 2 lines

I think it's a violation of CONFIG_ARCH_HAS_GIGANTIC_PAGE. The existing folio_dump() code would not handle it correctly as well.

I'm trying to dig into history and when looking at commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") I understand that CONFIG_ARCH_HAS_GIGANTIC_PAGE is needed to be able to allocate gigantic pages at runtime. It is not needed to reserve gigantic pages at boottime.

Quoted 4 lines

Quoted 1 line

What am I missing ?

That CONFIG_ARCH_HAS_GIGANTIC_PAGE has nothing runtime-specific in its name.

Quoted 7 lines

In its name for sure, but the commit I mention says:

      On systems without CONTIG_ALLOC activated but that support gigantic pages,
      boottime reserved gigantic pages can not be freed at all.  This patch simply enables the possibility to hand back those pages to memory allocator.

Right, I think it was a historical artifact.

Quoted 17 lines

And one of the hunks is:

arch/arm64/Kconfig +0 -0

index 7f7fbd8bd9d5b..7a1aa53d188d3 100644
@@ -19,7 +19,7 @@ config ARM64

          select ARCH_HAS_FAST_MULTIPLIER select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_GCOV_PROFILE_ALL
-       select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC +       select ARCH_HAS_GIGANTIC_PAGE
          select ARCH_HAS_KCOV
          select ARCH_HAS_KEEPINITRD
          select ARCH_HAS_MEMBARRIER_SYNC_CORE

Quoted 3 lines

So I understand from the commit message that it was possible at that time to have gigantic pages without ARCH_HAS_GIGANTIC_PAGE as long as you didn't have to be able to free them during runtime.

Yes, I agree.

Quoted 9 lines

Quoted 5 lines

Can't we just select CONFIG_ARCH_HAS_GIGANTIC_PAGE for the relevant hugetlb config that allows for *gigantic pages*.

We probably can, but I'd really like to understand history and how we ended up in the situation we are now.
Because blind fixes often lead to more problems.

Yes, let's figure out how to to it cleanly.

Quoted 5 lines

If I follow things correctly I see a helper gigantic_page_supported() added by commit 944d9fec8d7a ("hugetlb: add support for gigantic page allocation at runtime").

Quoted 8 lines

And then commit 461a7184320a ("mm/hugetlb: introduce ARCH_HAS_GIGANTIC_PAGE") is added to wrap gigantic_page_supported()

Then commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") changed gigantic_page_supported() to gigantic_page_runtime_supported()

So where are we now ?

In

commit fae7d834c43ccdb9fcecaf4d0f33145d884b3e5c
Author: Matthew Wilcox (Oracle) <[email protected]> Date:   Tue Feb 27 19:23:31 2024 +0000

    mm: add __dump_folio()

We started assuming that a folio in the system (boottime, dynamic, whatever) has a maximum of MAX_FOLIO_NR_PAGES.

Any other interpretation doesn't make any sense for MAX_FOLIO_NR_PAGES.

So we have two questions:

1) How to teach MAX_FOLIO_NR_PAGES that hugetlb supports gigantic pages

2) How do we handle CONFIG_ARCH_HAS_GIGANTIC_PAGE

We have the following options

(A) Rename existing CONFIG_ARCH_HAS_GIGANTIC_PAGE to something else that is clearer and add a new CONFIG_ARCH_HAS_GIGANTIC_PAGE.

(B) Rename existing CONFIG_ARCH_HAS_GIGANTIC_PAGE -> to something else that is clearer and derive somehow else that hugetlb in that config supports gigantic pages.

(c) Just use CONFIG_ARCH_HAS_GIGANTIC_PAGE if hugetlb on an architecture supports gigantic pages.

I don't quite see why an architecture should be able to opt in into dynamically allocating+freeing gigantic pages. That's just CONTIG_ALLOC magic and not some arch-specific thing IIRC.

Note that in mm/hugetlb.c it is

    #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
    #ifdef CONFIG_CONTIG_ALLOC

Meaning that at least the allocation side is guarded by CONTIG_ALLOC.

So I think (C) is just the right thing to do.

fs/Kconfig +0 -0

index 0bfdaecaa8775..12c11eb9279d3 100644

Le 09/10/2025 à 12:27, David Hildenbrand a écrit :

Quoted 193 lines · Christophe Leroy

Quoted 86 lines

Le 09/10/2025 à 11:20, David Hildenbrand a écrit :

Quoted 78 lines · Christophe Leroy

Quoted 77 lines

Le 09/10/2025 à 10:14, David Hildenbrand a écrit :

Quoted 64 lines · Christophe Leroy

Quoted 52 lines

Le 09/10/2025 à 09:22, David Hildenbrand a écrit :

Quoted 24 lines · Christophe Leroy

Quoted 23 lines

Hi David,

Le 01/09/2025 à 17:03, David Hildenbrand a écrit :

Quoted 20 lines

mm/hugetlb.c +0 -0

index 1e777cc51ad04..d3542e92a712e 100644
@@ -4657,6 +4657,7 @@ static int __init hugetlb_init(void)
           BUILD_BUG_ON(sizeof_field(struct page, private) *

BITS_PER_BYTE <
                  __NR_HPAGEFLAGS);
+    BUILD_BUG_ON_INVALID(HUGETLB_PAGE_ORDER > MAX_FOLIO_ORDER);
          if (!hugepages_supported()) {
              if (hugetlb_max_hstate ||
default_hstate_max_huge_pages)

mm/hugetlb.c +1 -0

@@ -4740,6 +4741,7 @@ void __init hugetlb_add_hstate(unsigned int
 order)
           }
           BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
           BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
+    WARN_ON(order > MAX_FOLIO_ORDER);
           h = &hstates[hugetlb_max_hstate++];
           __mutex_init(&h->resize_lock, "resize mutex", &h-

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

resize_key);

          h->order = order;

We end up registering hugetlb folios that are bigger than MAX_FOLIO_ORDER. So we have to figure out how a config can trigger that
(and if we have to support that).

MAX_FOLIO_ORDER is defined as:

#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
#define MAX_FOLIO_ORDER        PUD_ORDER
#else
#define MAX_FOLIO_ORDER        MAX_PAGE_ORDER
#endif

MAX_PAGE_ORDER is the limit for dynamic creation of hugepages via /sys/kernel/mm/hugepages/ but bigger pages can be created at boottime with kernel boot parameters without CONFIG_ARCH_HAS_GIGANTIC_PAGE:

      hugepagesz=64m hugepages=1 hugepagesz=256m hugepages=1

Gives:

Quoted 10 lines

HugeTLB: registered 1.00 GiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 1.00 GiB page
HugeTLB: registered 64.0 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 64.0 MiB page
HugeTLB: registered 256 MiB page size, pre-allocated 1 pages
HugeTLB: 0 KiB vmemmap can be freed for a 256 MiB page
HugeTLB: registered 4.00 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 4.00 MiB page
HugeTLB: registered 16.0 MiB page size, pre-allocated 0 pages
HugeTLB: 0 KiB vmemmap can be freed for a 16.0 MiB page

Quoted 3 lines

I think it's a violation of CONFIG_ARCH_HAS_GIGANTIC_PAGE. The existing
folio_dump() code would not handle it correctly as well.

I'm trying to dig into history and when looking at commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") I understand that CONFIG_ARCH_HAS_GIGANTIC_PAGE is needed to be able to allocate gigantic pages at runtime. It is not needed to reserve gigantic pages at boottime.

Quoted 4 lines

Quoted 1 line

What am I missing ?

That CONFIG_ARCH_HAS_GIGANTIC_PAGE has nothing runtime-specific in its name.

Quoted 8 lines

In its name for sure, but the commit I mention says:

      On systems without CONTIG_ALLOC activated but that support gigantic
pages,
      boottime reserved gigantic pages can not be freed at all.  This
patch
      simply enables the possibility to hand back those pages to memory allocator.

Right, I think it was a historical artifact.

Quoted 17 lines

And one of the hunks is:

arch/arm64/Kconfig +1 -1

index 7f7fbd8bd9d5b..7a1aa53d188d3 100644
@@ -19,7 +19,7 @@ config ARM64
           select ARCH_HAS_FAST_MULTIPLIER
           select ARCH_HAS_FORTIFY_SOURCE
           select ARCH_HAS_GCOV_PROFILE_ALL
-       select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
+       select ARCH_HAS_GIGANTIC_PAGE
           select ARCH_HAS_KCOV
           select ARCH_HAS_KEEPINITRD
           select ARCH_HAS_MEMBARRIER_SYNC_CORE

Quoted 3 lines

So I understand from the commit message that it was possible at that time to have gigantic pages without ARCH_HAS_GIGANTIC_PAGE as long as you didn't have to be able to free them during runtime.

Yes, I agree.

Quoted 9 lines

Quoted 5 lines

Can't we just select CONFIG_ARCH_HAS_GIGANTIC_PAGE for the relevant hugetlb config that allows for *gigantic pages*.

We probably can, but I'd really like to understand history and how we ended up in the situation we are now.
Because blind fixes often lead to more problems.

Yes, let's figure out how to to it cleanly.

Quoted 5 lines

If I follow things correctly I see a helper gigantic_page_supported() added by commit 944d9fec8d7a ("hugetlb: add support for gigantic page allocation at runtime").

Quoted 8 lines

And then commit 461a7184320a ("mm/hugetlb: introduce ARCH_HAS_GIGANTIC_PAGE") is added to wrap gigantic_page_supported()

Then commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration") changed gigantic_page_supported() to gigantic_page_runtime_supported()

So where are we now ?

In

commit fae7d834c43ccdb9fcecaf4d0f33145d884b3e5c
Author: Matthew Wilcox (Oracle) <[email protected]> Date:   Tue Feb 27 19:23:31 2024 +0000

    mm: add __dump_folio()

We started assuming that a folio in the system (boottime, dynamic, whatever)
has a maximum of MAX_FOLIO_NR_PAGES.

Any other interpretation doesn't make any sense for MAX_FOLIO_NR_PAGES.

So we have two questions:

1) How to teach MAX_FOLIO_NR_PAGES that hugetlb supports gigantic pages

2) How do we handle CONFIG_ARCH_HAS_GIGANTIC_PAGE

We have the following options

(A) Rename existing CONFIG_ARCH_HAS_GIGANTIC_PAGE to something else that is clearer and add a new CONFIG_ARCH_HAS_GIGANTIC_PAGE.

(B) Rename existing CONFIG_ARCH_HAS_GIGANTIC_PAGE -> to something else that is
clearer and derive somehow else that hugetlb in that config supports gigantic pages.

(c) Just use CONFIG_ARCH_HAS_GIGANTIC_PAGE if hugetlb on an architecture supports gigantic pages.

I don't quite see why an architecture should be able to opt in into dynamically
allocating+freeing gigantic pages. That's just CONTIG_ALLOC magic and not some
arch-specific thing IIRC.

Note that in mm/hugetlb.c it is

Quoted 4 lines

      #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
      #ifdef CONFIG_CONTIG_ALLOC

Meaning that at least the allocation side is guarded by CONTIG_ALLOC.

Yes but not the freeing since commit 4eb0716e868e ("hugetlb: allow to free gigantic pages regardless of the configuration")

Quoted 12 lines

So I think (C) is just the right thing to do.

fs/Kconfig +0 -0

index 0bfdaecaa8775..12c11eb9279d3 100644
@@ -283,6 +283,8 @@ config HUGETLB_PMD_PAGE_TABLE_SHARING
          def_bool HUGETLB_PAGE
          depends on ARCH_WANT_HUGE_PMD_SHARE && SPLIT_PMD_PTLOCKS
 

fs/Kconfig config HUGETLB_PMD_PAGE_TABLE_SHARING · line 289

Quoted 7 lines

fs/Kconfig +2 -0

+# An architecture must select this option if there is any mechanism
 (esp. hugetlb)
+# could obtain gigantic folios.
   config ARCH_HAS_GIGANTIC_PAGE
          bool

I gave it a try. That's not enough, it fixes the problem for 64 Mbytes pages and 256 Mbytes pages, but not for 1 Gbytes pages.

Max folio is defined by PUD_ORDER, but PUD_SIZE is 256 Mbytes so we need to make MAX_FOLIO larger. Do we change it to P4D_ORDER or is it too much ? P4D_SIZE is 128 Gbytes

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/gup.c | 25 ++++++++-----------------
  1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -17

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
  #ifdef CONFIG_MMU
 
  #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-			   unsigned long addr, unsigned long end,
-			   struct page **pages)
-{
-	int nr;
-
-	page += (addr & (sz - 1)) >> PAGE_SHIFT;
-	for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-		pages[nr] = page++;
-
-	return nr;
-}
-
  /**
   * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
   * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	if (pmd_special(orig))
  		return 0;
 
-	page = pmd_page(orig);
-	refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
 
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	}
 
  	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
  	folio_set_referenced(folio);
  	return 1;
  }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
  	if (pud_special(orig))
  		return 0;
 
-	page = pud_page(orig);
-	refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
 
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

Quoted 61 lines · David Hildenbrand

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/gup.c | 25 ++++++++-----------------
  1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -15

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
  #ifdef CONFIG_MMU
    #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-               unsigned long addr, unsigned long end,
-               struct page **pages)
-{
-    int nr;
-
-    page += (addr & (sz - 1)) >> PAGE_SHIFT;
-    for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-        pages[nr] = page++;
-
-    return nr;
-}
-
  /**
   * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
   * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
      if (pmd_special(orig))
          return 0;
  -    page = pmd_page(orig);
-    refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+    refs = (end - addr) >> PAGE_SHIFT;
+    page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
        folio = try_grab_folio_fast(page, refs, flags);
      if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
      }
        *nr += refs;
+    for (; refs; refs--)
+        *(pages++) = page++;
      folio_set_referenced(folio);
      return 1;
  }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
      if (pud_special(orig))
          return 0;
  -    page = pud_page(orig);
-    refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+    refs = (end - addr) >> PAGE_SHIFT;
+    page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
        folio = try_grab_folio_fast(page, refs, flags);
      if (!folio)

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3025

Quoted 7 lines

mm/gup.c +2 -0

@@ -3030,6 +3019,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
      }
        *nr += refs;
+    for (; refs; refs--)
+        *(pages++) = page++;
      folio_set_referenced(folio);
      return 1;
  }

Okay, this code is nasty. We should rework this code to just return the nr and receive a the proper pages pointer, getting rid of the "*nr" parameter.

For the time being, the following should do the trick:

commit bfd07c995814354f6b66c5b6a72e96a7aa9fb73b (HEAD -> nth_page)

Author: David Hildenbrand <[email protected]>
Date:   Fri Sep 5 08:38:43 2025 +0200

fixup: mm/gup: remove record_subpages()
pages is not adjusted by the caller, but idnexed by existing *nr.

Signed-off-by David Hildenbrand <[email protected]>

mm/gup.c +1 -0

index 010fe56f6e132..22420f2069ee1 100644
@@ -2981,6 +2981,7 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
                return 0;
        }
 
+       pages += *nr;
        *nr += refs;
        for (; refs; refs--)
                *(pages++) = page++;

Quoted 7 lines · David Hildenbrand

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

Quoted 54 lines

---
  mm/gup.c | 25 ++++++++-----------------
  1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -17

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
  #ifdef CONFIG_MMU
  #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-			   unsigned long addr, unsigned long end,
-			   struct page **pages)
-{
-	int nr;
-
-	page += (addr & (sz - 1)) >> PAGE_SHIFT;
-	for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-		pages[nr] = page++;
-
-	return nr;
-}
-
  /**
   * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
   * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	if (pmd_special(orig))
  		return 0;
-	page = pmd_page(orig);
-	refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	}
  	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
  	folio_set_referenced(folio);
  	return 1;
  }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
  	if (pud_special(orig))
  		return 0;
-	page = pud_page(orig);
-	refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3025

Quoted 7 lines

mm/gup.c +2 -0

@@ -3030,6 +3019,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
  	}
  	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
  	folio_set_referenced(folio);
  	return 1;
  }

Okay, this code is nasty. We should rework this code to just return the nr and receive a the proper pages pointer, getting rid of the "*nr" parameter.

For the time being, the following should do the trick:

commit bfd07c995814354f6b66c5b6a72e96a7aa9fb73b (HEAD -> nth_page)

Author: David Hildenbrand <[email protected]>
Date:   Fri Sep 5 08:38:43 2025 +0200

fixup: mm/gup: remove record_subpages()
pages is not adjusted by the caller, but idnexed by existing *nr.

Signed-off-by David Hildenbrand <[email protected]>

mm/gup.c +1 -0

index 010fe56f6e132..22420f2069ee1 100644
@@ -2981,6 +2981,7 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
                return 0;
        }
+       pages += *nr;
        *nr += refs;
        for (; refs; refs--)
                *(pages++) = page++;

mm/gup.c +0 -1

--

Cheers

David / dhildenb

Quoted 8 lines · David Hildenbrand

Quoted 7 lines · David Hildenbrand

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

So strange I thought I looked at this...!

Quoted 87 lines

Quoted 54 lines

---
   mm/gup.c | 25 ++++++++-----------------
   1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -17

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
   #ifdef CONFIG_MMU
   #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-			   unsigned long addr, unsigned long end,
-			   struct page **pages)
-{
-	int nr;
-
-	page += (addr & (sz - 1)) >> PAGE_SHIFT;
-	for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-		pages[nr] = page++;
-
-	return nr;
-}
-
   /**
    * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
    * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
   	if (pmd_special(orig))
   		return 0;
-	page = pmd_page(orig);
-	refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
   	folio = try_grab_folio_fast(page, refs, flags);
   	if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
   	}
   	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
   	folio_set_referenced(folio);
   	return 1;
   }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
   	if (pud_special(orig))
   		return 0;
-	page = pud_page(orig);
-	refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
   	folio = try_grab_folio_fast(page, refs, flags);
   	if (!folio)

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3025

Quoted 7 lines

mm/gup.c +2 -0

@@ -3030,6 +3019,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
   	}
   	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
   	folio_set_referenced(folio);
   	return 1;
   }

Okay, this code is nasty. We should rework this code to just return the nr and receive a the proper pages pointer, getting rid of the "*nr" parameter.

For the time being, the following should do the trick:

commit bfd07c995814354f6b66c5b6a72e96a7aa9fb73b (HEAD -> nth_page)

Author: David Hildenbrand <[email protected]>
Date:   Fri Sep 5 08:38:43 2025 +0200

fixup: mm/gup: remove record_subpages()
pages is not adjusted by the caller, but idnexed by existing *nr.

Signed-off-by David Hildenbrand <[email protected]>

mm/gup.c +1 -0

index 010fe56f6e132..22420f2069ee1 100644
@@ -2981,6 +2981,7 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
                 return 0;
         }
+       pages += *nr;
         *nr += refs;
         for (; refs; refs--)
                 *(pages++) = page++;

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3030

Quoted 6 lines

mm/gup.c +1 -0

@@ -3024,6 +3025,7 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
                 return 0;
         }
+       pages += *nr;
         *nr += refs;
         for (; refs; refs--)
                 *(pages++) = page++;

This looks correct.

But.

This is VERY nasty. Before we'd call record_subpages() with pages + *nr, where it was clear we were offsetting by this, now we're making things imo way more confusing.

This makes me less in love with this approach to be honest.

But perhaps it's the least worst thing for now until we can do a bigger refactor...

So since this seems correct to me, and for the sake of moving things forward (was this one patch dropped from mm-new or does mm-new just have an old version? Confused):

Reviewed-by Lorenzo Stoakes <[email protected]>

For this patch obviously with the fix applied.

But can we PLEASE revisit this :)

Quoted 61 lines · David Hildenbrand

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/gup.c | 25 ++++++++-----------------
  1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -17

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
  #ifdef CONFIG_MMU
  #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-			   unsigned long addr, unsigned long end,
-			   struct page **pages)
-{
-	int nr;
-
-	page += (addr & (sz - 1)) >> PAGE_SHIFT;
-	for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-		pages[nr] = page++;
-
-	return nr;
-}
-
  /**
   * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
   * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	if (pmd_special(orig))
  		return 0;
-	page = pmd_page(orig);
-	refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	}
  	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
  	folio_set_referenced(folio);
  	return 1;
  }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
  	if (pud_special(orig))
  		return 0;
-	page = pud_page(orig);
-	refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3025

Quoted 7 lines

mm/gup.c +2 -0

@@ -3030,6 +3019,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
  	}
  	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
  	folio_set_referenced(folio);
  	return 1;
  }

Okay, this code is nasty. We should rework this code to just return the nr and receive a the proper pages pointer, getting rid of the "*nr" parameter.

For the time being, the following should do the trick:

commit bfd07c995814354f6b66c5b6a72e96a7aa9fb73b (HEAD -> nth_page)

Author: David Hildenbrand <[email protected]>
Date:   Fri Sep 5 08:38:43 2025 +0200

fixup: mm/gup: remove record_subpages()
pages is not adjusted by the caller, but idnexed by existing *nr.

Signed-off-by David Hildenbrand <[email protected]>

mm/gup.c +1 -0

index 010fe56f6e132..22420f2069ee1 100644
@@ -2981,6 +2981,7 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
                return 0;
        }
+       pages += *nr;
        *nr += refs;
        for (; refs; refs--)
                *(pages++) = page++;

Quoted 95 lines · David Hildenbrand

Quoted 61 lines · David Hildenbrand

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

---
   mm/gup.c | 25 ++++++++-----------------
   1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -17

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
   #ifdef CONFIG_MMU
   #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-			   unsigned long addr, unsigned long end,
-			   struct page **pages)
-{
-	int nr;
-
-	page += (addr & (sz - 1)) >> PAGE_SHIFT;
-	for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-		pages[nr] = page++;
-
-	return nr;
-}
-
   /**
    * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
    * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
   	if (pmd_special(orig))
   		return 0;
-	page = pmd_page(orig);
-	refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
   	folio = try_grab_folio_fast(page, refs, flags);
   	if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
   	}
   	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
   	folio_set_referenced(folio);
   	return 1;
   }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
   	if (pud_special(orig))
   		return 0;
-	page = pud_page(orig);
-	refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
   	folio = try_grab_folio_fast(page, refs, flags);
   	if (!folio)

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3025

Quoted 7 lines

mm/gup.c +2 -0

@@ -3030,6 +3019,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
   	}
   	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
   	folio_set_referenced(folio);
   	return 1;
   }

Okay, this code is nasty. We should rework this code to just return the nr and receive a the proper pages pointer, getting rid of the "*nr" parameter.

For the time being, the following should do the trick:

commit bfd07c995814354f6b66c5b6a72e96a7aa9fb73b (HEAD -> nth_page)

Author: David Hildenbrand <[email protected]>
Date:   Fri Sep 5 08:38:43 2025 +0200

fixup: mm/gup: remove record_subpages()
pages is not adjusted by the caller, but idnexed by existing *nr.

Signed-off-by David Hildenbrand <[email protected]>

mm/gup.c +1 -0

index 010fe56f6e132..22420f2069ee1 100644
@@ -2981,6 +2981,7 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
                 return 0;
         }
+       pages += *nr;
         *nr += refs;
         for (; refs; refs--)
                 *(pages++) = page++;

@@ -3024,6 +3025,7 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,

mm/gup.c static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr, · line 2987

Quoted 6 lines

mm/gup.c +1 -0

                 return 0;
         }
+       pages += *nr;
         *nr += refs;
         for (; refs; refs--)
                 *(pages++) = page++;

Can this get folded in soon? This bug is causing crashes in AF_ALG too.

Quoted 9 lines

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3030

Quoted 7 lines

mm/gup.c +1 -0

@@ -3024,6 +3025,7 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
                 return 0;
         }
+       pages += *nr;
         *nr += refs;
         for (; refs; refs--)
                 *(pages++) = page++;

Can this get folded in soon? This bug is causing crashes in AF_ALG too.

Andrew immediately dropped the original patch, so it's gone from mm-unstable and should be gone from next soon (today?).

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/gup.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -17

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
 #ifdef CONFIG_MMU
 
 #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-			   unsigned long addr, unsigned long end,
-			   struct page **pages)
-{
-	int nr;
-
-	page += (addr & (sz - 1)) >> PAGE_SHIFT;
-	for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-		pages[nr] = page++;
-
-	return nr;
-}
-
 /**
  * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
  * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
 	if (pmd_special(orig))
 		return 0;
 
-	page = pmd_page(orig);
-	refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
 
 	folio = try_grab_folio_fast(page, refs, flags);
 	if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
 	}
 
 	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
 	folio_set_referenced(folio);
 	return 1;
 }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
 	if (pud_special(orig))
 		return 0;
 
-	page = pud_page(orig);
-	refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
 
 	folio = try_grab_folio_fast(page, refs, flags);
 	if (!folio)

mm/gup.c +0 -0

 	folio_set_referenced(folio);
 	return 1;
 }

Quoted 69 lines · David Hildenbrand

We can just cleanup the code by calculating the #refs earlier, so we can just inline what remains of record_subpages().

Calculate the number of references/pages ahead of times, and record them only once all our tests passed.

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/gup.c | 25 ++++++++-----------------
  1 file changed, 8 insertions(+), 17 deletions(-)

mm/gup.c +6 -17

index c10cd969c1a3b..f0f4d1a68e094 100644
@@ -484,19 +484,6 @@ static inline void mm_set_has_pinned_flag(struct mm_struct *mm)
  #ifdef CONFIG_MMU
 
  #ifdef CONFIG_HAVE_GUP_FAST
-static int record_subpages(struct page *page, unsigned long sz,
-			   unsigned long addr, unsigned long end,
-			   struct page **pages)
-{
-	int nr;
-
-	page += (addr & (sz - 1)) >> PAGE_SHIFT;
-	for (nr = 0; addr != end; nr++, addr += PAGE_SIZE)
-		pages[nr] = page++;
-
-	return nr;
-}
-
  /**
   * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
   * @page:  pointer to page to be grabbed

@@ -2967,8 +2954,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	if (pmd_special(orig))
  		return 0;
 
-	page = pmd_page(orig);
-	refs = record_subpages(page, PMD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
 
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

@@ -2989,6 +2976,8 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
  	}
 
  	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;
  	folio_set_referenced(folio);
  	return 1;
  }

@@ -3007,8 +2996,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
  	if (pud_special(orig))
  		return 0;
 
-	page = pud_page(orig);
-	refs = record_subpages(page, PUD_SIZE, addr, end, pages + *nr);
+	refs = (end - addr) >> PAGE_SHIFT;
+	page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
 
  	folio = try_grab_folio_fast(page, refs, flags);
  	if (!folio)

@@ -3030,6 +3019,8 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
  	}
 

mm/gup.c static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr, · line 3023

Quoted 3 lines

mm/gup.c +2 -0

  	*nr += refs;
+	for (; refs; refs--)
+		*(pages++) = page++;

Hi David,

Probably a similar sentiment as Lorenzo here...the above diffs make the code *worse* to read. In fact, I recall adding record_subpages() here long ago, specifically to help clarify what was going on.

Now it's been returned to it's original, cryptic form.

Just my take on it, for whatever that's worth. :)

   	pmdp = pmd_offset_lockless(pudp, pud, addr);

mm/gup.c +5 -6

@@ -3046,23 +3041,21 @@ static int gup_fast_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr,
 
   		next = pmd_addr_end(addr, end);
   		if (!pmd_present(pmd))
-			return 0;
+			break;
 
-		if (unlikely(pmd_leaf(pmd))) {
-			/* See gup_fast_pte_range() */
-			if (pmd_protnone(pmd))
-				return 0;
+		if (unlikely(pmd_leaf(pmd)))
+			cur_nr_pages = gup_fast_pmd_leaf(pmd, pmdp, addr, next, flags, pages);
+		else
+			cur_nr_pages = gup_fast_pte_range(pmd, pmdp, addr, next, flags, pages);
 
-			if (!gup_fast_pmd_leaf(pmd, pmdp, addr, next, flags,

Quoted 1 line · John Hubbard

On 9/1/25 8:03 AM, David Hildenbrand wrote:

Starting with "as Willy keeps explaining, the concept of subpages do not exist and ending with "why do we fill out the array even on failure".

:)

Quoted 3 lines

Now it's been returned to it's original, cryptic form.

The code in the caller was so uncryptic that both me and Lorenzo missed that magical addition. :P

Quoted 1 line

Just my take on it, for whatever that's worth. :)

As always, appreciated.

I could of course keep the simple loop in some "record_folio_pages" function and clean up what I dislike about record_subpages().

But I much rather want the call chain to be cleaned up instead, if possible.

Roughly, what I am thinking (limiting it to pte+pmd case) about is the following:

From d6d6d21dbf435d8030782a627175e36e6c7b2dfb Mon Sep 17 00:00:00 2001
From: David Hildenbrand <[email protected]>
Date: Sat, 6 Sep 2025 08:33:42 +0200
Subject: [PATCH] tmp

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/gup.c | 79 ++++++++++++++++++++++++++------------------------------
  1 file changed, 36 insertions(+), 43 deletions(-)

mm/gup.c +3 -4

index 22420f2069ee1..98907ead749c0 100644
@@ -2845,12 +2845,11 @@ static void __maybe_unused 
 gup_fast_undo_dev_pagemap(int *nr, int nr_start,
   * also check pmd here to make sure pmd doesn't change (corresponds to
   * pmdp_collapse_flush() in the THP collapse code path).
   */
-static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
-        unsigned long end, unsigned int flags, struct page **pages,
-        int *nr)
+static unsigned long gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, 
 unsigned long addr,
+        unsigned long end, unsigned int flags, struct page **pages)
  {
      struct dev_pagemap *pgmap = NULL;
-    int ret = 0;
+    unsigned long nr_pages = 0;
      pte_t *ptep, *ptem;

     ptem = ptep = pte_offset_map(&pmd, addr);

mm/gup.c +3 -7

@@ -2908,24 +2907,20 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t 
 *pmdp, unsigned long addr,
           * details.
           */
          if (flags & FOLL_PIN) {
-            ret = arch_make_folio_accessible(folio);
-            if (ret) {
+            if (arch_make_folio_accessible(folio)) {
                  gup_put_folio(folio, 1, flags);
                  goto pte_unmap;
              }
          }
          folio_set_referenced(folio);
-        pages[*nr] = page;
-        (*nr)++;
+        pages[nr_pages++] = page;
      } while (ptep++, addr += PAGE_SIZE, addr != end);
 
-    ret = 1;
-
  pte_unmap:
      if (pgmap)
          put_dev_pagemap(pgmap);
      pte_unmap(ptem);
-    return ret;
+    return nr_pages;
  }
  #else

@@ -2938,21 +2933,24 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t 
 *pmdp, unsigned long addr,
   * get_user_pages_fast_only implementation that can pin pages. Thus 

it's still
  * useful to have gup_fast_pmd_leaf even if we can't operate on ptes. */

mm/gup.c +1 -3

-static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
-        unsigned long end, unsigned int flags, struct page **pages,
-        int *nr)
+static unsigned long gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, 

unsigned long addr,
+        unsigned long end, unsigned int flags, struct page **pages)
 {
     return 0;
 }
 #endif /* CONFIG_ARCH_HAS_PTE_SPECIAL */

mm/gup.c +1 -3

-static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
-        unsigned long end, unsigned int flags, struct page **pages,
-        int *nr)
+static unsigned long gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, 

unsigned long addr,
+        unsigned long end, unsigned int flags, struct page **pages)
 {
+    const unsigned long nr_pages = (end - addr) >> PAGE_SHIFT;
     struct page *page;
     struct folio *folio;
-    int refs;
+    unsigned long i;

mm/gup.c +10 -9

+
+    /* See gup_fast_pte_range() */
+    if (pmd_protnone(orig))
+        return 0;
 
      if (!pmd_access_permitted(orig, flags & FOLL_WRITE))
          return 0;

@@ -2960,33 +2958,30 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t 
 *pmdp, unsigned long addr,
      if (pmd_special(orig))
          return 0;
 
-    refs = (end - addr) >> PAGE_SHIFT;
      page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
 
-    folio = try_grab_folio_fast(page, refs, flags);
+    folio = try_grab_folio_fast(page, nr_pages, flags);
      if (!folio)
          return 0;
 
      if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) {
-        gup_put_folio(folio, refs, flags);
+        gup_put_folio(folio, nr_pages, flags);
          return 0;
      }
 
      if (!gup_fast_folio_allowed(folio, flags)) {
-        gup_put_folio(folio, refs, flags);
+        gup_put_folio(folio, nr_pages, flags);
          return 0;
      }
      if (!pmd_write(orig) && gup_must_unshare(NULL, flags, &folio- 
 >page)) {
-        gup_put_folio(folio, refs, flags);
+        gup_put_folio(folio, nr_pages, flags);
          return 0;
      }
 
-    pages += *nr;
-    *nr += refs;
-    for (; refs; refs--)
+    for (i = 0; i < nr_pages; i++)
          *(pages++) = page++;
      folio_set_referenced(folio);
-    return 1;
+    return nr_pages;
  }

 static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,

mm/gup.c +4 -4

@@ -3033,11 +3028,11 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t 
 *pudp, unsigned long addr,
      return 1;
  }
 
-static int gup_fast_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr,
-        unsigned long end, unsigned int flags, struct page **pages,
-        int *nr)
+static unsigned long gup_fast_pmd_range(pud_t *pudp, pud_t pud, 
 unsigned long addr,
+        unsigned long end, unsigned int flags, struct page **pages)
  {
-    unsigned long next;
+    unsigned long cur_nr_pages, next;
+    unsigned long nr_pages = 0;
      pmd_t *pmdp;

     pmdp = pmd_offset_lockless(pudp, pud, addr);

mm/gup.c +10 -12

@@ -3046,23 +3041,21 @@ static int gup_fast_pmd_range(pud_t *pudp, pud_t 
 pud, unsigned long addr,
 
          next = pmd_addr_end(addr, end);
          if (!pmd_present(pmd))
-            return 0;
+            break;
 
-        if (unlikely(pmd_leaf(pmd))) {
-            /* See gup_fast_pte_range() */
-            if (pmd_protnone(pmd))
-                return 0;
+        if (unlikely(pmd_leaf(pmd)))
+            cur_nr_pages = gup_fast_pmd_leaf(pmd, pmdp, addr, next, 
 flags, pages);
+        else
+            cur_nr_pages = gup_fast_pte_range(pmd, pmdp, addr, next, 
 flags, pages);
 
-            if (!gup_fast_pmd_leaf(pmd, pmdp, addr, next, flags,
-                pages, nr))
-                return 0;
+        nr_pages += cur_nr_pages;
+        pages += cur_nr_pages;
 
-        } else if (!gup_fast_pte_range(pmd, pmdp, addr, next, flags,
-                           pages, nr))
-            return 0;
+        if (nr_pages != (next - addr) >> PAGE_SIZE)
+            break;
      } while (pmdp++, addr = next, addr != end);
 
-    return 1;
+    return nr_pages;

 }

 static int gup_fast_pud_range(p4d_t *p4dp, p4d_t p4d, unsigned long addr,

Quoted 2 lines

Roughly, what I am thinking (limiting it to pte+pmd case) about is the following:

The code below looks much cleaner, that's great!

Quoted 4 lines · John Hubbard

Probably a similar sentiment as Lorenzo here...the above diffs make the code *worse* to read. In fact, I recall adding record_subpages() here long ago, specifically to help clarify what was going on.

Well, there is a lot I dislike about record_subpages() to go back there. Starting with "as Willy keeps explaining, the concept of subpages do not exist and ending with "why do we fill out the array even on failure".

:)

Quoted 3 lines

Now it's been returned to it's original, cryptic form.

The code in the caller was so uncryptic that both me and Lorenzo missed that magical addition. :P

Quoted 2 lines

Just my take on it, for whatever that's worth. :)

As always, appreciated.

I could of course keep the simple loop in some "record_folio_pages" function and clean up what I dislike about record_subpages().

But I much rather want the call chain to be cleaned up instead, if possible.

Roughly, what I am thinking (limiting it to pte+pmd case) about is the following:

From d6d6d21dbf435d8030782a627175e36e6c7b2dfb Mon Sep 17 00:00:00 2001
From: David Hildenbrand <[email protected]>
Date: Sat, 6 Sep 2025 08:33:42 +0200
Subject: [PATCH] tmp

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/gup.c | 79 ++++++++++++++++++++++++++------------------------------
 1 file changed, 36 insertions(+), 43 deletions(-)

mm/gup.c +33 -39

index 22420f2069ee1..98907ead749c0 100644
@@ -2845,12 +2845,11 @@ static void __maybe_unused gup_fast_undo_dev_pagemap(int *nr, int nr_start,
  * also check pmd here to make sure pmd doesn't change (corresponds to
  * pmdp_collapse_flush() in the THP collapse code path).
  */
-static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
-		unsigned long end, unsigned int flags, struct page **pages,
-		int *nr)
+static unsigned long gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
+		unsigned long end, unsigned int flags, struct page **pages)
 {
 	struct dev_pagemap *pgmap = NULL;
-	int ret = 0;
+	unsigned long nr_pages = 0;
 	pte_t *ptep, *ptem;
 	ptem = ptep = pte_offset_map(&pmd, addr);

@@ -2908,24 +2907,20 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
 		 * details.
 		 */
 		if (flags & FOLL_PIN) {
-			ret = arch_make_folio_accessible(folio);
-			if (ret) {
+			if (arch_make_folio_accessible(folio)) {
 				gup_put_folio(folio, 1, flags);
 				goto pte_unmap;
 			}
 		}
 		folio_set_referenced(folio);
-		pages[*nr] = page;
-		(*nr)++;
+		pages[nr_pages++] = page;
 	} while (ptep++, addr += PAGE_SIZE, addr != end);
-	ret = 1;
-
 pte_unmap:
 	if (pgmap)
 		put_dev_pagemap(pgmap);
 	pte_unmap(ptem);
-	return ret;
+	return nr_pages;
 }
 #else

@@ -2938,21 +2933,24 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
  * get_user_pages_fast_only implementation that can pin pages. Thus it's still
  * useful to have gup_fast_pmd_leaf even if we can't operate on ptes.
  */
-static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
-		unsigned long end, unsigned int flags, struct page **pages,
-		int *nr)
+static unsigned long gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
+		unsigned long end, unsigned int flags, struct page **pages)
 {
 	return 0;
 }
 #endif /* CONFIG_ARCH_HAS_PTE_SPECIAL */
-static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
-		unsigned long end, unsigned int flags, struct page **pages,
-		int *nr)
+static unsigned long gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
+		unsigned long end, unsigned int flags, struct page **pages)
 {
+	const unsigned long nr_pages = (end - addr) >> PAGE_SHIFT;
 	struct page *page;
 	struct folio *folio;
-	int refs;
+	unsigned long i;
+
+	/* See gup_fast_pte_range() */
+	if (pmd_protnone(orig))
+		return 0;
 	if (!pmd_access_permitted(orig, flags & FOLL_WRITE))
 		return 0;

@@ -2960,33 +2958,30 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
 	if (pmd_special(orig))
 		return 0;
-	refs = (end - addr) >> PAGE_SHIFT;
 	page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
-	folio = try_grab_folio_fast(page, refs, flags);
+	folio = try_grab_folio_fast(page, nr_pages, flags);
 	if (!folio)
 		return 0;
 	if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) {
-		gup_put_folio(folio, refs, flags);
+		gup_put_folio(folio, nr_pages, flags);
 		return 0;
 	}
 	if (!gup_fast_folio_allowed(folio, flags)) {
-		gup_put_folio(folio, refs, flags);
+		gup_put_folio(folio, nr_pages, flags);
 		return 0;
 	}
 	if (!pmd_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) {
-		gup_put_folio(folio, refs, flags);
+		gup_put_folio(folio, nr_pages, flags);
 		return 0;
 	}
-	pages += *nr;
-	*nr += refs;
-	for (; refs; refs--)
+	for (i = 0; i < nr_pages; i++)
 		*(pages++) = page++;
 	folio_set_referenced(folio);
-	return 1;
+	return nr_pages;
 }
 static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,

@@ -3033,11 +3028,11 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
 	return 1;
 }
-static int gup_fast_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr,
-		unsigned long end, unsigned int flags, struct page **pages,
-		int *nr)
+static unsigned long gup_fast_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr,
+		unsigned long end, unsigned int flags, struct page **pages)
 {
-	unsigned long next;
+	unsigned long cur_nr_pages, next;
+	unsigned long nr_pages = 0;
 	pmd_t *pmdp;
 	pmdp = pmd_offset_lockless(pudp, pud, addr);

@@ -3046,23 +3041,21 @@ static int gup_fast_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr,
 		next = pmd_addr_end(addr, end);
 		if (!pmd_present(pmd))
-			return 0;
+			break;
-		if (unlikely(pmd_leaf(pmd))) {
-			/* See gup_fast_pte_range() */
-			if (pmd_protnone(pmd))
-				return 0;
+		if (unlikely(pmd_leaf(pmd)))
+			cur_nr_pages = gup_fast_pmd_leaf(pmd, pmdp, addr, next, flags, pages);
+		else
+			cur_nr_pages = gup_fast_pte_range(pmd, pmdp, addr, next, flags, pages);
-			if (!gup_fast_pmd_leaf(pmd, pmdp, addr, next, flags,
-				pages, nr))
-				return 0;
+		nr_pages += cur_nr_pages;
+		pages += cur_nr_pages;

mm/gup.c +0 -1

--

2.50.1

Oh, I might even have found a bug moving away from that questionable "ret==1 means success" handling in gup_fast_pte_range()? Will have to double-check, but likely the following is the right thing to do.

From 8f48b25ef93e7ef98611fd58ec89384ad5171782 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <[email protected]>
Date: Sat, 6 Sep 2025 08:46:45 +0200
Subject: [PATCH] mm/gup: fix handling of errors from
arch_make_folio_accessible() in follow_page_pte()

In case we call arch_make_folio_accessible() and it fails, we would incorrectly return a value that is "!= 0" to the caller, indicating that we pinned all requested pages and that the caller can keep going.

follow_page_pte() is not supposed to return error values, but instead 0 on failure and 1 on success.

That is of course wrong, because the caller will just keep going pinning more pages. If we happen to pin a page afterwards, we're in trouble, because we essentially skipped some pages.

Fixes f28d43636d6f ("mm/gup/writeback: add callbacks for inaccessible pages")

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/gup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

mm/gup.c +0 -0

index 22420f2069ee1..cff226ec0ee7d 100644

mm/gup.c +0 -1

 				gup_put_folio(folio, 1, flags);
 				goto pte_unmap;
 			}
--

2.50.1

--
Cheers

David / dhildenb