[PATCH] um/mm: get max_low_pfn from memblock

[PATCH] um/mm: get max_low_pfn from memblock

[Wei Yang]

Current calculation of max_low_pfn is introduced in commit af84eab20891
("[PATCH] uml: fix LVM crash"). It is intended to set max_low_pfn to the
same value as max_pfn.

But I am not sure why the max_pfn is set to totalram_pages, which
represents the number of usable pages in system instead of an absolute
page frame number. (The change history stops there.)

While we can get the maximum page frame number from memblock, this looks
more reasonable than setting to totalram_pages.

Also this would help changing totalram_pages accounting, since we plan
to move the accounting into __free_pages_core(). With this change,
totalram_pages may not represent the total usable pages at this point,
since some pages would be deferred initialized.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Jason Lunz <lunz@falooley.org>
CC: Jeff Dike <jdike@linux.intel.com>
Cc: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
Cc: Alasdair G Kergon <agk@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Mike Rapoport (IBM) <rppt@kernel.org>
CC: David Hildenbrand <david@redhat.com>

---
A simple UML bootup test looks good.
---
 arch/um/kernel/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
index ca91accd64fc..ca682879e28f 100644
--- a/arch/um/kernel/mem.c
+++ b/arch/um/kernel/mem.c
@@ -73,7 +73,7 @@ void __init mem_init(void)
 
 	/* this will put all low memory onto the freelists */
 	memblock_free_all();
-	max_low_pfn = totalram_pages();
+	max_low_pfn = PFN_DOWN(memblock_end_of_DRAM());
 	max_pfn = max_low_pfn;
 	kmalloc_ok = 1;
 }
-- 
2.34.1

Re: [PATCH] um/mm: get max_low_pfn from memblock

[David Hildenbrand]

On 14.06.24 03:58, Wei Yang wrote:
> Current calculation of max_low_pfn is introduced in commit af84eab20891
> ("[PATCH] uml: fix LVM crash"). It is intended to set max_low_pfn to the
> same value as max_pfn.
> 
> But I am not sure why the max_pfn is set to totalram_pages, which
> represents the number of usable pages in system instead of an absolute
> page frame number. (The change history stops there.)
> 
> While we can get the maximum page frame number from memblock, this looks
> more reasonable than setting to totalram_pages.
> 
> Also this would help changing totalram_pages accounting, since we plan
> to move the accounting into __free_pages_core(). With this change,
> totalram_pages may not represent the total usable pages at this point,
> since some pages would be deferred initialized.

Question is if deferred page init is even a thing on UM. But it certainly looks odd+suspiciously wrong to rely on totalram_pages().

> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Jason Lunz <lunz@falooley.org>
> CC: Jeff Dike <jdike@linux.intel.com>
> Cc: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> Cc: Alasdair G Kergon <agk@redhat.com>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Mike Rapoport (IBM) <rppt@kernel.org>
> CC: David Hildenbrand <david@redhat.com>
> 
> ---
> A simple UML bootup test looks good.
> ---
>   arch/um/kernel/mem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
> index ca91accd64fc..ca682879e28f 100644
> --- a/arch/um/kernel/mem.c
> +++ b/arch/um/kernel/mem.c
> @@ -73,7 +73,7 @@ void __init mem_init(void)
>   
>   	/* this will put all low memory onto the freelists */
>   	memblock_free_all();
> -	max_low_pfn = totalram_pages();
> +	max_low_pfn = PFN_DOWN(memblock_end_of_DRAM());
>   	max_pfn = max_low_pfn;
>   	kmalloc_ok = 1;
>   }

Matches what a bunch of other archs do.

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


Randomly staring at other users:

arch/loongarch/kernel/numa.c:   max_low_pfn = PHYS_PFN(memblock_end_of_DRAM());
arch/loongarch/kernel/setup.c:  max_low_pfn = PFN_PHYS(memblock_end_of_DRAM());

What? the latter cannot possibly be right, no? Looks odd at least.
Only applies to CONFIG_OF_EARLY_FLATTREE. CCing loongarch maintainer.

-- 
Cheers,

David / dhildenb

Re: [PATCH] um/mm: get max_low_pfn from memblock

[Mike Rapoport]

On Fri, Jun 14, 2024 at 09:31:59AM +0200, David Hildenbrand wrote:
> On 14.06.24 03:58, Wei Yang wrote:
> > Current calculation of max_low_pfn is introduced in commit af84eab20891
> > ("[PATCH] uml: fix LVM crash"). It is intended to set max_low_pfn to the
> > same value as max_pfn.
> > 
> > But I am not sure why the max_pfn is set to totalram_pages, which
> > represents the number of usable pages in system instead of an absolute
> > page frame number. (The change history stops there.)
> > 
> > While we can get the maximum page frame number from memblock, this looks
> > more reasonable than setting to totalram_pages.
> > 
> > Also this would help changing totalram_pages accounting, since we plan
> > to move the accounting into __free_pages_core(). With this change,
> > totalram_pages may not represent the total usable pages at this point,
> > since some pages would be deferred initialized.
> 
> Question is if deferred page init is even a thing on UM. But it certainly looks odd+suspiciously wrong to rely on totalram_pages().
 
As long as there is no HIGHMEM, 

max_pfn = max_low_pfn = PFN_DOWN(memblock_end_of_DRAM())

should be ok.
 
> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > CC: Jason Lunz <lunz@falooley.org>
> > CC: Jeff Dike <jdike@linux.intel.com>
> > Cc: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> > Cc: Alasdair G Kergon <agk@redhat.com>
> > Cc: Jens Axboe <jens.axboe@oracle.com>
> > CC: Andrew Morton <akpm@linux-foundation.org>
> > CC: Mike Rapoport (IBM) <rppt@kernel.org>
> > CC: David Hildenbrand <david@redhat.com>
> > 
> > ---
> > A simple UML bootup test looks good.
> > ---
> >   arch/um/kernel/mem.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
> > index ca91accd64fc..ca682879e28f 100644
> > --- a/arch/um/kernel/mem.c
> > +++ b/arch/um/kernel/mem.c
> > @@ -73,7 +73,7 @@ void __init mem_init(void)
> >   	/* this will put all low memory onto the freelists */
> >   	memblock_free_all();
> > -	max_low_pfn = totalram_pages();
> > +	max_low_pfn = PFN_DOWN(memblock_end_of_DRAM());

This assignment seem redundant as there is already

	max_low_pfn = min_low_pfn + (map_size >> PAGE_SHIFT);

in setup_physmem

> >   	max_pfn = max_low_pfn;
> >   	kmalloc_ok = 1;
> >   }
> 
> Matches what a bunch of other archs do.
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 
> 
> Randomly staring at other users:
> 
> arch/loongarch/kernel/numa.c:   max_low_pfn = PHYS_PFN(memblock_end_of_DRAM());
> arch/loongarch/kernel/setup.c:  max_low_pfn = PFN_PHYS(memblock_end_of_DRAM());
> 
> What? the latter cannot possibly be right, no? Looks odd at least.
> Only applies to CONFIG_OF_EARLY_FLATTREE. CCing loongarch maintainer.
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] um/mm: get max_low_pfn from memblock

[Wei Yang]

On Fri, Jun 14, 2024 at 10:51:32AM +0300, Mike Rapoport wrote:
>On Fri, Jun 14, 2024 at 09:31:59AM +0200, David Hildenbrand wrote:
>> On 14.06.24 03:58, Wei Yang wrote:
>> > Current calculation of max_low_pfn is introduced in commit af84eab20891
>> > ("[PATCH] uml: fix LVM crash"). It is intended to set max_low_pfn to the
>> > same value as max_pfn.
>> > 
>> > But I am not sure why the max_pfn is set to totalram_pages, which
>> > represents the number of usable pages in system instead of an absolute
>> > page frame number. (The change history stops there.)
>> > 
>> > While we can get the maximum page frame number from memblock, this looks
>> > more reasonable than setting to totalram_pages.
>> > 
>> > Also this would help changing totalram_pages accounting, since we plan
>> > to move the accounting into __free_pages_core(). With this change,
>> > totalram_pages may not represent the total usable pages at this point,
>> > since some pages would be deferred initialized.
>> 
>> Question is if deferred page init is even a thing on UM. But it certainly looks odd+suspiciously wrong to rely on totalram_pages().
> 
>As long as there is no HIGHMEM, 
>
>max_pfn = max_low_pfn = PFN_DOWN(memblock_end_of_DRAM())
>
>should be ok.
> 
>> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> > CC: Jason Lunz <lunz@falooley.org>
>> > CC: Jeff Dike <jdike@linux.intel.com>
>> > Cc: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
>> > Cc: Alasdair G Kergon <agk@redhat.com>
>> > Cc: Jens Axboe <jens.axboe@oracle.com>
>> > CC: Andrew Morton <akpm@linux-foundation.org>
>> > CC: Mike Rapoport (IBM) <rppt@kernel.org>
>> > CC: David Hildenbrand <david@redhat.com>
>> > 
>> > ---
>> > A simple UML bootup test looks good.
>> > ---
>> >   arch/um/kernel/mem.c | 2 +-
>> >   1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
>> > index ca91accd64fc..ca682879e28f 100644
>> > --- a/arch/um/kernel/mem.c
>> > +++ b/arch/um/kernel/mem.c
>> > @@ -73,7 +73,7 @@ void __init mem_init(void)
>> >   	/* this will put all low memory onto the freelists */
>> >   	memblock_free_all();
>> > -	max_low_pfn = totalram_pages();
>> > +	max_low_pfn = PFN_DOWN(memblock_end_of_DRAM());
>
>This assignment seem redundant as there is already
>
>	max_low_pfn = min_low_pfn + (map_size >> PAGE_SHIFT);
>
>in setup_physmem
>

Looks right, I added some log and shows they are the same.

Thanks

>> >   	max_pfn = max_low_pfn;
>> >   	kmalloc_ok = 1;
>> >   }
>> 
>> Matches what a bunch of other archs do.
>> 
>> Acked-by: David Hildenbrand <david@redhat.com>
>> 
>> 
>> Randomly staring at other users:
>> 
>> arch/loongarch/kernel/numa.c:   max_low_pfn = PHYS_PFN(memblock_end_of_DRAM());
>> arch/loongarch/kernel/setup.c:  max_low_pfn = PFN_PHYS(memblock_end_of_DRAM());
>> 
>> What? the latter cannot possibly be right, no? Looks odd at least.
>> Only applies to CONFIG_OF_EARLY_FLATTREE. CCing loongarch maintainer.
>> 
>> -- 
>> Cheers,
>> 
>> David / dhildenb
>> 
>
>-- 
>Sincerely yours,
>Mike.

-- 
Wei Yang
Help you, Help me