[PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Ruihan Li]

Currently, memmap_init initializes pfn_hole with 0 instead of
ARCH_PFN_OFFSET. Then init_unavailable_range will start iterating each
page from the page at address zero to the first available page, but it
won't do anything for pages below ARCH_PFN_OFFSET because pfn_valid
won't pass.

If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
kernel is used as a library and loaded at a very high address), the
pointless iteration for pages below ARCH_PFN_OFFSET will take a very
long time, and the kernel will look stuck at boot time.

This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
avoids the problematic and useless iteration mentioned above.

This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
unavailable struct pages").

Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
---
Link to v1:
 - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
Changes since v1:
 - Removed the unnecessary Fixes tag.
 - Fixed the build issue for CONFIG_SPARSEMEM.

 mm/mm_init.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mm/mm_init.c b/mm/mm_init.c
index 84f14fa12..a697a83ff 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -969,6 +969,15 @@ static void __init memmap_init(void)
 	unsigned long hole_pfn = 0;
 	int i, j, zone_id = 0, nid;
 
+#ifdef CONFIG_FLATMEM
+	/*
+	 * Pages below ARCH_PFN_OFFSET are invalid as far as pfn_valid is
+	 * concerned, so don't waste time iterating on them when looking
+	 * for holes.
+	 */
+	hole_pfn = ARCH_PFN_OFFSET;
+#endif
+
 	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
 		struct pglist_data *node = NODE_DATA(nid);
 
-- 
2.49.0

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Andrew Morton]

On Sat, 19 Apr 2025 20:28:01 +0800 Ruihan Li <lrh2000@pku.edu.cn> wrote:

> If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> kernel is used as a library and loaded at a very high address), the
> pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> long time, and the kernel will look stuck at boot time.
> 
> This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> avoids the problematic and useless iteration mentioned above.
> 
> This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> unavailable struct pages").
> 
> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> ---
> Link to v1:
>  - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> Changes since v1:
>  - Removed the unnecessary Fixes tag.

Why was the Fixes: considered unnecessary?  It seems to be useful
information?

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Ruihan Li]

Hi Andrew,

Thanks for your review and for taking the patch in the -mm tree!

On Sun, Apr 20, 2025 at 01:57:09PM -0700, Andrew Morton wrote:
> On Sat, 19 Apr 2025 20:28:01 +0800 Ruihan Li <lrh2000@pku.edu.cn> wrote:
> 
> > If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> > kernel is used as a library and loaded at a very high address), the
> > pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> > long time, and the kernel will look stuck at boot time.
> > 
> > This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> > avoids the problematic and useless iteration mentioned above.
> > 
> > This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> > unavailable struct pages").
> > 
> > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > ---
> > Link to v1:
> >  - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> > Changes since v1:
> >  - Removed the unnecessary Fixes tag.
> 
> Why was the Fixes: considered unnecessary?  It seems to be useful
> information?
> 

To clarify, I only removed it because I got comments saying that this
patch doesn't really fix a BUG (well, that statement depends on how one
defines a BUG: it's a real BUG in my scenario, but maybe not a BUG for
many other scenarios):
https://lore.kernel.org/linux-mm/aANN0rwxcajUtFXs@kernel.org/

Since I don't know if there are other rules that apply here, I followed
the review comments to remove it.

Another possibility is that I misunderstood Mike's original comment?
Sorry if that happens.

Thanks,
Ruihan Li

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Mike Rapoport]

On Sun, Apr 20, 2025 at 01:57:09PM -0700, Andrew Morton wrote:
> On Sat, 19 Apr 2025 20:28:01 +0800 Ruihan Li <lrh2000@pku.edu.cn> wrote:
> 
> > If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> > kernel is used as a library and loaded at a very high address), the
> > pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> > long time, and the kernel will look stuck at boot time.
> > 
> > This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> > avoids the problematic and useless iteration mentioned above.
> > 
> > This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> > unavailable struct pages").
> > 
> > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > ---
> > Link to v1:
> >  - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> > Changes since v1:
> >  - Removed the unnecessary Fixes tag.
> 
> Why was the Fixes: considered unnecessary?  It seems to be useful
> information?

I didn't think it was important enough for AUTOSEL to pick it. 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Andrew Morton]

On Mon, 21 Apr 2025 19:35:25 +0300 Mike Rapoport <rppt@kernel.org> wrote:

> On Sun, Apr 20, 2025 at 01:57:09PM -0700, Andrew Morton wrote:
> > On Sat, 19 Apr 2025 20:28:01 +0800 Ruihan Li <lrh2000@pku.edu.cn> wrote:
> > 
> > > If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> > > kernel is used as a library and loaded at a very high address), the
> > > pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> > > long time, and the kernel will look stuck at boot time.
> > > 
> > > This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> > > avoids the problematic and useless iteration mentioned above.
> > > 
> > > This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> > > unavailable struct pages").
> > > 
> > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > > ---
> > > Link to v1:
> > >  - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> > > Changes since v1:
> > >  - Removed the unnecessary Fixes tag.
> > 
> > Why was the Fixes: considered unnecessary?  It seems to be useful
> > information?
> 
> I didn't think it was important enough for AUTOSEL to pick it. 

They've been asked not to backport MM patches unless they have an
explicit cc:stable.

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Mike Rapoport]

On Sat, Apr 19, 2025 at 08:28:01PM +0800, Ruihan Li wrote:
> Currently, memmap_init initializes pfn_hole with 0 instead of
> ARCH_PFN_OFFSET. Then init_unavailable_range will start iterating each
> page from the page at address zero to the first available page, but it
> won't do anything for pages below ARCH_PFN_OFFSET because pfn_valid
> won't pass.
> 
> If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> kernel is used as a library and loaded at a very high address), the
> pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> long time, and the kernel will look stuck at boot time.
> 
> This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> avoids the problematic and useless iteration mentioned above.
> 
> This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> unavailable struct pages").
> 
> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> ---
> Link to v1:
>  - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> Changes since v1:
>  - Removed the unnecessary Fixes tag.
>  - Fixed the build issue for CONFIG_SPARSEMEM.
> 
>  mm/mm_init.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/mm/mm_init.c b/mm/mm_init.c
> index 84f14fa12..a697a83ff 100644
> --- a/mm/mm_init.c
> +++ b/mm/mm_init.c
> @@ -969,6 +969,15 @@ static void __init memmap_init(void)
>  	unsigned long hole_pfn = 0;
>  	int i, j, zone_id = 0, nid;
>  
> +#ifdef CONFIG_FLATMEM
> +	/*
> +	 * Pages below ARCH_PFN_OFFSET are invalid as far as pfn_valid is
> +	 * concerned, so don't waste time iterating on them when looking
> +	 * for holes.
> +	 */
> +	hole_pfn = ARCH_PFN_OFFSET;
> +#endif
> +

I'd prefer a solution for both FLATMEM and SPARSMEM. 

David Woodhouse proposed a for_each_valid_pfn() a while ago:

https://lore.kernel.org/all/20250404155959.3442111-1-dwmw2@infradead.org

It can be used in init_unavailable_range() and will essentially skip the
unpopulated memory map.

>  	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
>  		struct pglist_data *node = NODE_DATA(nid);
>  
> -- 
> 2.49.0
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Ruihan Li]

Hi Mike,

Thanks for your review!

On Mon, Apr 21, 2025 at 07:41:43PM +0300, Mike Rapoport wrote:
> On Sat, Apr 19, 2025 at 08:28:01PM +0800, Ruihan Li wrote:
> > Currently, memmap_init initializes pfn_hole with 0 instead of
> > ARCH_PFN_OFFSET. Then init_unavailable_range will start iterating each
> > page from the page at address zero to the first available page, but it
> > won't do anything for pages below ARCH_PFN_OFFSET because pfn_valid
> > won't pass.
> > 
> > If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> > kernel is used as a library and loaded at a very high address), the
> > pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> > long time, and the kernel will look stuck at boot time.
> > 
> > This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> > avoids the problematic and useless iteration mentioned above.
> > 
> > This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> > unavailable struct pages").
> > 
> > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > ---
> > Link to v1:
> >  - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> > Changes since v1:
> >  - Removed the unnecessary Fixes tag.
> >  - Fixed the build issue for CONFIG_SPARSEMEM.
> > 
> >  mm/mm_init.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > index 84f14fa12..a697a83ff 100644
> > --- a/mm/mm_init.c
> > +++ b/mm/mm_init.c
> > @@ -969,6 +969,15 @@ static void __init memmap_init(void)
> >  	unsigned long hole_pfn = 0;
> >  	int i, j, zone_id = 0, nid;
> >  
> > +#ifdef CONFIG_FLATMEM
> > +	/*
> > +	 * Pages below ARCH_PFN_OFFSET are invalid as far as pfn_valid is
> > +	 * concerned, so don't waste time iterating on them when looking
> > +	 * for holes.
> > +	 */
> > +	hole_pfn = ARCH_PFN_OFFSET;
> > +#endif
> > +
> 
> I'd prefer a solution for both FLATMEM and SPARSMEM. 
> 
> David Woodhouse proposed a for_each_valid_pfn() a while ago:
> 
> https://lore.kernel.org/all/20250404155959.3442111-1-dwmw2@infradead.org
> 
> It can be used in init_unavailable_range() and will essentially skip the
> unpopulated memory map.

for_each_valid_pfn sounds much better. Thanks for your input.

However, the problem is that David's patch is not showing up in the
mainline, so what can I do to move forward with my patch?

Perhaps you mean that I should wait until David's patch is merged and
send another patch to fix the problem?

> 
> >  	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
> >  		struct pglist_data *node = NODE_DATA(nid);
> >  
> > -- 
> > 2.49.0
> > 
> 
> -- 
> Sincerely yours,
> Mike.

Thanks,
Ruihan Li

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[Andrew Morton]

On Tue, 22 Apr 2025 17:08:06 +0800 Ruihan Li <lrh2000@pku.edu.cn> wrote:

> Hi Mike,
> 
> Thanks for your review!
> 
> On Mon, Apr 21, 2025 at 07:41:43PM +0300, Mike Rapoport wrote:
> > On Sat, Apr 19, 2025 at 08:28:01PM +0800, Ruihan Li wrote:
> > > Currently, memmap_init initializes pfn_hole with 0 instead of
> > > ARCH_PFN_OFFSET. Then init_unavailable_range will start iterating each
> > > page from the page at address zero to the first available page, but it
> > > won't do anything for pages below ARCH_PFN_OFFSET because pfn_valid
> > > won't pass.
> > > 
> > > If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> > > kernel is used as a library and loaded at a very high address), the
> > > pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> > > long time, and the kernel will look stuck at boot time.
> > > 
> > > This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> > > avoids the problematic and useless iteration mentioned above.
> > > 
> > > This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> > > unavailable struct pages").
> > > 
> > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > > ---
> > > Link to v1:
> > >  - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> > > Changes since v1:
> > >  - Removed the unnecessary Fixes tag.
> > >  - Fixed the build issue for CONFIG_SPARSEMEM.
> > > 
> > >  mm/mm_init.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > > index 84f14fa12..a697a83ff 100644
> > > --- a/mm/mm_init.c
> > > +++ b/mm/mm_init.c
> > > @@ -969,6 +969,15 @@ static void __init memmap_init(void)
> > >  	unsigned long hole_pfn = 0;
> > >  	int i, j, zone_id = 0, nid;
> > >  
> > > +#ifdef CONFIG_FLATMEM
> > > +	/*
> > > +	 * Pages below ARCH_PFN_OFFSET are invalid as far as pfn_valid is
> > > +	 * concerned, so don't waste time iterating on them when looking
> > > +	 * for holes.
> > > +	 */
> > > +	hole_pfn = ARCH_PFN_OFFSET;
> > > +#endif
> > > +
> > 
> > I'd prefer a solution for both FLATMEM and SPARSMEM. 
> > 
> > David Woodhouse proposed a for_each_valid_pfn() a while ago:
> > 
> > https://lore.kernel.org/all/20250404155959.3442111-1-dwmw2@infradead.org
> > 
> > It can be used in init_unavailable_range() and will essentially skip the
> > unpopulated memory map.
> 
> for_each_valid_pfn sounds much better. Thanks for your input.
> 
> However, the problem is that David's patch is not showing up in the
> mainline, so what can I do to move forward with my patch?
> 
> Perhaps you mean that I should wait until David's patch is merged and
> send another patch to fix the problem?

(cc David)

> > 
> > >  	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
> > >  		struct pglist_data *node = NODE_DATA(nid);
> > >  
> > > -- 
> > > 2.49.0
> > > 
> > 
> > -- 
> > Sincerely yours,
> > Mike.
> 
> Thanks,
> Ruihan Li

Re: [PATCH v2] mm/mm_init: Don't iterate pages below ARCH_PFN_OFFSET

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3184 bytes --]

On Tue, 2025-04-22 at 13:20 -0700, Andrew Morton wrote:
> On Tue, 22 Apr 2025 17:08:06 +0800 Ruihan Li <lrh2000@pku.edu.cn> wrote:
> 
> > Hi Mike,
> > 
> > Thanks for your review!
> > 
> > On Mon, Apr 21, 2025 at 07:41:43PM +0300, Mike Rapoport wrote:
> > > On Sat, Apr 19, 2025 at 08:28:01PM +0800, Ruihan Li wrote:
> > > > Currently, memmap_init initializes pfn_hole with 0 instead of
> > > > ARCH_PFN_OFFSET. Then init_unavailable_range will start iterating each
> > > > page from the page at address zero to the first available page, but it
> > > > won't do anything for pages below ARCH_PFN_OFFSET because pfn_valid
> > > > won't pass.
> > > > 
> > > > If ARCH_PFN_OFFSET is very large (e.g., something like 2^64-2GiB if the
> > > > kernel is used as a library and loaded at a very high address), the
> > > > pointless iteration for pages below ARCH_PFN_OFFSET will take a very
> > > > long time, and the kernel will look stuck at boot time.
> > > > 
> > > > This commit sets the initial value of pfn_hole to ARCH_PFN_OFFSET, which
> > > > avoids the problematic and useless iteration mentioned above.
> > > > 
> > > > This problem has existed since commit 907ec5fca3dc ("mm: zero remaining
> > > > unavailable struct pages").
> > > > 
> > > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > > > ---
> > > > Link to v1:
> > > >   - https://lore.kernel.org/linux-mm/20250418162727.1535335-1-lrh2000@pku.edu.cn/
> > > > Changes since v1:
> > > >   - Removed the unnecessary Fixes tag.
> > > >   - Fixed the build issue for CONFIG_SPARSEMEM.
> > > > 
> > > >   mm/mm_init.c | 9 +++++++++
> > > >   1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > > > index 84f14fa12..a697a83ff 100644
> > > > --- a/mm/mm_init.c
> > > > +++ b/mm/mm_init.c
> > > > @@ -969,6 +969,15 @@ static void __init memmap_init(void)
> > > >   	unsigned long hole_pfn = 0;
> > > >   	int i, j, zone_id = 0, nid;
> > > >   
> > > > +#ifdef CONFIG_FLATMEM
> > > > +	/*
> > > > +	 * Pages below ARCH_PFN_OFFSET are invalid as far as pfn_valid is
> > > > +	 * concerned, so don't waste time iterating on them when looking
> > > > +	 * for holes.
> > > > +	 */
> > > > +	hole_pfn = ARCH_PFN_OFFSET;
> > > > +#endif
> > > > +
> > > 
> > > I'd prefer a solution for both FLATMEM and SPARSMEM. 
> > > 
> > > David Woodhouse proposed a for_each_valid_pfn() a while ago:
> > > 
> > > https://lore.kernel.org/all/20250404155959.3442111-1-dwmw2@infradead.org
> > > 
> > > It can be used in init_unavailable_range() and will essentially skip the
> > > unpopulated memory map.
> > 
> > for_each_valid_pfn sounds much better. Thanks for your input.
> > 
> > However, the problem is that David's patch is not showing up in the
> > mainline, so what can I do to move forward with my patch?
> > 
> > Perhaps you mean that I should wait until David's patch is merged and
> > send another patch to fix the problem?
> 
> (cc David)

Want to add a patch on top of my tree at
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/for_each_valid_pfn
and I'll send it on with the rest?


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]