[RFC] arm64: mm: update max_pfn after memory hotplug

Commit Message

Chris Goldsworthy Sept. 23, 2021, 10:54 p.m. UTC

From: Sudarshan Rajagopalan <[e-mail]>

After new memory blocks have been hotplugged, max_pfn and max_low_pfn
needs updating to reflect on new PFNs being hot added to system.

Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
Signed-off-by: Chris Goldsworthy <[e-mail]>
---
 arch/arm64/mm/mmu.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Florian Fainelli Sept. 24, 2021, 2:47 a.m. UTC | #1

On 9/23/2021 3:54 PM, Chris Goldsworthy wrote:
> From: Sudarshan Rajagopalan <[e-mail]>
> 
> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> needs updating to reflect on new PFNs being hot added to system.
> 
> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> Signed-off-by: Chris Goldsworthy <[e-mail]>
> ---
>   arch/arm64/mm/mmu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index cfd9deb..fd85b51 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>   	if (ret)
>   		__remove_pgd_mapping(swapper_pg_dir,
>   				     __phys_to_virt(start), size);
> +	else {
> +		max_pfn = PFN_UP(start + size);
> +		max_low_pfn = max_pfn;
> +	}

This is a drive by review, but it got me thinking about your changes a bit:

- if you raise max_pfn when you hotplug memory, don't you need to lower 
it when you hot unplug memory as well?

- suppose that you have a platform which maps physical memory into the 
CPU's address space at 0x00_4000_0000 (1GB offset) and the kernel boots 
with 2GB of DRAM plugged by default. At that point we have not 
registered a swiotlb because we have less than 4GB of addressable 
physical memory, there is no IOMMU in that system, it's a happy world. 
Now assume that we plug an additional 2GB of DRAM into that system 
adjacent to the previous 2GB, from 0x00_C0000_0000 through 
0x14_0000_0000, now we have physical addresses above 4GB, but we still 
don't have a swiotlb, some of our DMA_BIT_MASK(32) peripherals are going 
to be unable to DMA from that hot plugged memory, but they could if we 
had a swiotlb.

- now let's go even further but this is very contrived. Assume that the 
firmware has somewhat created a reserved memory region with a 'no-map' 
attribute thus indicating it does not want a struct page to be created 
for a specific PFN range, is it valid to "blindly" raise max_pfn if that 
region were to be at the end of the just hot-plugged memory?

David Hildenbrand Sept. 24, 2021, 8:17 a.m. UTC | #2

On 24.09.21 04:47, Florian Fainelli wrote:
> 
> 
> On 9/23/2021 3:54 PM, Chris Goldsworthy wrote:
>> From: Sudarshan Rajagopalan <[e-mail]>
>>
>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>> needs updating to reflect on new PFNs being hot added to system.
>>
>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>> ---
>>    arch/arm64/mm/mmu.c | 5 +++++
>>    1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index cfd9deb..fd85b51 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>    	if (ret)
>>    		__remove_pgd_mapping(swapper_pg_dir,
>>    				     __phys_to_virt(start), size);
>> +	else {
>> +		max_pfn = PFN_UP(start + size);
>> +		max_low_pfn = max_pfn;
>> +	}
> 
> This is a drive by review, but it got me thinking about your changes a bit:
> 
> - if you raise max_pfn when you hotplug memory, don't you need to lower
> it when you hot unplug memory as well?

The issue with lowering is that you actually have to do some search to 
figure out the actual value -- and it's not really worth the trouble. 
Raising the limit is easy.

With memory hotunplug, anybody wanting to take a look at a "struct page" 
via a pfn has to do a pfn_to_online_page() either way. That will fail if 
there isn't actually a memmap anymore because the memory has been 
unplugged. So "max_pfn" is actually rather a hint what maximum pfn to 
look at, and it can be bigger than it actually is.

The a look at the example usage in fs/proc/page.c:kpageflags_read()

pfn_to_online_page() will simply fail and stable_page_flags() will 
indicate a KPF_NOPAGE.

Just like we would have a big memory hole now at the end of memory.

> 
> - suppose that you have a platform which maps physical memory into the
> CPU's address space at 0x00_4000_0000 (1GB offset) and the kernel boots
> with 2GB of DRAM plugged by default. At that point we have not
> registered a swiotlb because we have less than 4GB of addressable
> physical memory, there is no IOMMU in that system, it's a happy world.
> Now assume that we plug an additional 2GB of DRAM into that system
> adjacent to the previous 2GB, from 0x00_C0000_0000 through
> 0x14_0000_0000, now we have physical addresses above 4GB, but we still
> don't have a swiotlb, some of our DMA_BIT_MASK(32) peripherals are going
> to be unable to DMA from that hot plugged memory, but they could if we
> had a swiotlb.

That's why platforms that hotplug memory should indicate the maximum 
possible PFN via some mechanism during boot. On x86-64 (and IIRC also 
arm64 now), this is done via the ACPI SRAT.

And that's where "max_possible_pfn" and "max_pfn" differ. See 
drivers/acpi/numa/srat.c:acpi_numa_memory_affinity_init():

	max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));$


Using max_possible_pfn, the OS can properly setup the swiotlb, even 
thought it wouldn't currently be required when just looking at max_pfn.

I documented that for virtio-mem in
	https://virtio-mem.gitlab.io/user-guide/user-guide-linux.html
"swiotlb and DMA memory".

> 
> - now let's go even further but this is very contrived. Assume that the
> firmware has somewhat created a reserved memory region with a 'no-map'
> attribute thus indicating it does not want a struct page to be created
> for a specific PFN range, is it valid to "blindly" raise max_pfn if that
> region were to be at the end of the just hot-plugged memory?

no-map means that no direct mapping is to be created, right? We would 
still have a memmap IIRC, and the pages are PG_reserved.

Again, I think this is very similar to just having no-map regions like 
random memory holes within the existing memory layout.


What Chris proposes here is very similar to 
arch/x86/mm/init_64.c:update_end_of_memory_vars() called during 
arch_add_memory()->add_pages() on x86-64.

Chris Goldsworthy Sept. 24, 2021, 8:52 p.m. UTC | #3

Thanks for the response David.

On Fri, Sep 24, 2021 at 10:17:46AM +0200, David Hildenbrand wrote:
> no-map means that no direct mapping is to be created, right? We would still
> have a memmap IIRC, and the pages are PG_reserved.
> 
> Again, I think this is very similar to just having no-map regions like
> random memory holes within the existing memory layout.

For those curious, see __reserved_mem_alloc_size() >
early_init_dt_alloc_reserved_memory_arch() > memblock_mark_nomap() - the
'no-map' attribute is read in __reserved_mem_alloc_size() and the pre-requisite
steps need to have the relevant struct pages marked as PG_reserved 
are taken in memblock_mark_nomap().

> What Chris proposes here is very similar to
> arch/x86/mm/init_64.c:update_end_of_memory_vars() called during
> arch_add_memory()->add_pages() on x86-64.
> 

For other's reference, the patch was derived from what x86 is doing with max_pfn
(such that we also set max_low_pfn as is done in arm64's mm/init.c.

Sudarshan Rajagopalan Sept. 25, 2021, 12:36 a.m. UTC | #4

+Anshuman

Thanks for the responses. Do we see any issues with having this patch go 
for merge? I agree with David that 'max_pfn' is just a hint about 
maximum possible PFN that can go. So it doesn't need updating when 
memory is hotunplugged out.

Also regarding comment about possible reserved memory region with 
"no-map" attribute - I'm not sure if we such reserved memory allocations 
are allowed post early init when memory blocks are hot added later on 
after arm64_memblock_init is done? These are all done during setup_arch 
right?

We found this issue/bug where kernel inits with limited boot memory 
using 'mem=' and a driver hot-adds memory blocks during module init 
using add_memory_driver_managed and owns it. This makes 'max_pfn' out of 
sync and breaks /proc/kpageXXX kpageflags_read() functionality where any 
processes using pages beyond this outdated 'max_pfn', 
stable_page_flags() would simply return KPF_NOPAGE.

I think this is generic bug that exists with arm64 memory hotplug where 
any consumers of 'max_pfn' would break its functionality after memory 
blocks are hotplugged in after init. If this patch looks OK and be 
ACKed, we can have it in GKI. Other suggestions on proper fix or 
feedback are also welcome.

On 9/24/2021 1:17 AM, David Hildenbrand wrote:
> On 24.09.21 04:47, Florian Fainelli wrote:
>>
>>
>> On 9/23/2021 3:54 PM, Chris Goldsworthy wrote:
>>> From: Sudarshan Rajagopalan <[e-mail]>
>>>
>>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>>> needs updating to reflect on new PFNs being hot added to system.
>>>
>>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>>> ---
>>>    arch/arm64/mm/mmu.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>> index cfd9deb..fd85b51 100644
>>> --- a/arch/arm64/mm/mmu.c
>>> +++ b/arch/arm64/mm/mmu.c
>>> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 
>>> size,
>>>        if (ret)
>>>            __remove_pgd_mapping(swapper_pg_dir,
>>>                         __phys_to_virt(start), size);
>>> +    else {
>>> +        max_pfn = PFN_UP(start + size);
>>> +        max_low_pfn = max_pfn;
>>> +    }
>>
>> This is a drive by review, but it got me thinking about your changes 
>> a bit:
>>
>> - if you raise max_pfn when you hotplug memory, don't you need to lower
>> it when you hot unplug memory as well?
>
> The issue with lowering is that you actually have to do some search to 
> figure out the actual value -- and it's not really worth the trouble. 
> Raising the limit is easy.
>
> With memory hotunplug, anybody wanting to take a look at a "struct 
> page" via a pfn has to do a pfn_to_online_page() either way. That will 
> fail if there isn't actually a memmap anymore because the memory has 
> been unplugged. So "max_pfn" is actually rather a hint what maximum 
> pfn to look at, and it can be bigger than it actually is.
>
> The a look at the example usage in fs/proc/page.c:kpageflags_read()
>
> pfn_to_online_page() will simply fail and stable_page_flags() will 
> indicate a KPF_NOPAGE.
>
> Just like we would have a big memory hole now at the end of memory.
>
>>
>> - suppose that you have a platform which maps physical memory into the
>> CPU's address space at 0x00_4000_0000 (1GB offset) and the kernel boots
>> with 2GB of DRAM plugged by default. At that point we have not
>> registered a swiotlb because we have less than 4GB of addressable
>> physical memory, there is no IOMMU in that system, it's a happy world.
>> Now assume that we plug an additional 2GB of DRAM into that system
>> adjacent to the previous 2GB, from 0x00_C0000_0000 through
>> 0x14_0000_0000, now we have physical addresses above 4GB, but we still
>> don't have a swiotlb, some of our DMA_BIT_MASK(32) peripherals are going
>> to be unable to DMA from that hot plugged memory, but they could if we
>> had a swiotlb.
>
> That's why platforms that hotplug memory should indicate the maximum 
> possible PFN via some mechanism during boot. On x86-64 (and IIRC also 
> arm64 now), this is done via the ACPI SRAT.
>
> And that's where "max_possible_pfn" and "max_pfn" differ. See 
> drivers/acpi/numa/srat.c:acpi_numa_memory_affinity_init():
>
>     max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));$
>
>
> Using max_possible_pfn, the OS can properly setup the swiotlb, even 
> thought it wouldn't currently be required when just looking at max_pfn.
>
> I documented that for virtio-mem in
>     https://virtio-mem.gitlab.io/user-guide/user-guide-linux.html
> "swiotlb and DMA memory".
>
>>
>> - now let's go even further but this is very contrived. Assume that the
>> firmware has somewhat created a reserved memory region with a 'no-map'
>> attribute thus indicating it does not want a struct page to be created
>> for a specific PFN range, is it valid to "blindly" raise max_pfn if that
>> region were to be at the end of the just hot-plugged memory?
>
> no-map means that no direct mapping is to be created, right? We would 
> still have a memmap IIRC, and the pages are PG_reserved.
>
> Again, I think this is very similar to just having no-map regions like 
> random memory holes within the existing memory layout.
>
>
> What Chris proposes here is very similar to 
> arch/x86/mm/init_64.c:update_end_of_memory_vars() called during 
> arch_add_memory()->add_pages() on x86-64.
>

David Hildenbrand Sept. 27, 2021, 3:51 p.m. UTC | #5

On 24.09.21 00:54, Chris Goldsworthy wrote:
> From: Sudarshan Rajagopalan <[e-mail]>
> 
> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> needs updating to reflect on new PFNs being hot added to system.
> 
> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> Signed-off-by: Chris Goldsworthy <[e-mail]>
> ---
>   arch/arm64/mm/mmu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index cfd9deb..fd85b51 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>   	if (ret)
>   		__remove_pgd_mapping(swapper_pg_dir,
>   				     __phys_to_virt(start), size);
> +	else {
> +		max_pfn = PFN_UP(start + size);
> +		max_low_pfn = max_pfn;
> +	}
> +
>   	return ret;

Note: didn't verify if updating max_low_pfn is correct here.

Acked-by: David Hildenbrand <[e-mail]>

Georgi Djakov Sept. 27, 2021, 5:22 p.m. UTC | #6

On 9/24/2021 1:54 AM, Chris Goldsworthy wrote:
> From: Sudarshan Rajagopalan <[e-mail]>
> 
> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> needs updating to reflect on new PFNs being hot added to system.
> 
> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> Signed-off-by: Chris Goldsworthy <[e-mail]>

Thanks for the patch, Chris!

With this patch, the data in /proc/kpageflags appears to be correct and
memory tools like procrank work again on arm64 platforms.

Tested-by: Georgi Djakov <[e-mail]>

Maybe we should add fixes tag, as it has been broken since the following
commit:
Fixes: abec749facff ("fs/proc/page.c: allow inspection of last section 
and fix end detection")

Thanks,
Georgi

> ---
>   arch/arm64/mm/mmu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index cfd9deb..fd85b51 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>   	if (ret)
>   		__remove_pgd_mapping(swapper_pg_dir,
>   				     __phys_to_virt(start), size);
> +	else {
> +		max_pfn = PFN_UP(start + size);
> +		max_low_pfn = max_pfn;
> +	}
> +
>   	return ret;
>   }
>   
>

David Hildenbrand Sept. 27, 2021, 5:34 p.m. UTC | #7

On 27.09.21 19:22, Georgi Djakov wrote:
> On 9/24/2021 1:54 AM, Chris Goldsworthy wrote:
>> From: Sudarshan Rajagopalan <[e-mail]>
>>
>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>> needs updating to reflect on new PFNs being hot added to system.
>>
>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>> Signed-off-by: Chris Goldsworthy <[e-mail]>
> 
> Thanks for the patch, Chris!
> 
> With this patch, the data in /proc/kpageflags appears to be correct and
> memory tools like procrank work again on arm64 platforms.
> 
> Tested-by: Georgi Djakov <[e-mail]>
> 
> Maybe we should add fixes tag, as it has been broken since the following
> commit:
> Fixes: abec749facff ("fs/proc/page.c: allow inspection of last section
> and fix end detection")

Are you sure that that commit broke it?

I recall that we would naturally run into the limit, because

count = min_t(size_t, count, (max_pfn * KPMSIZE) - src);

wouldn't really do what you would expect either. But you could 
force-read beyond max_pfn, yes, because the count computation was just 
weird.


I think the real issue is not properly adjusting max_pfn in the first 
place when we introduced memoruy hotplug on arm64.

Georgi Djakov Sept. 27, 2021, 8 p.m. UTC | #8

On 9/27/2021 8:34 PM, David Hildenbrand wrote:
> On 27.09.21 19:22, Georgi Djakov wrote:
>> On 9/24/2021 1:54 AM, Chris Goldsworthy wrote:
>>> From: Sudarshan Rajagopalan <[e-mail]>
>>>
>>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>>> needs updating to reflect on new PFNs being hot added to system.
>>>
>>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>>
>> Thanks for the patch, Chris!
>>
>> With this patch, the data in /proc/kpageflags appears to be correct and
>> memory tools like procrank work again on arm64 platforms.
>>
>> Tested-by: Georgi Djakov <[e-mail]>
>>
>> Maybe we should add fixes tag, as it has been broken since the following
>> commit:
>> Fixes: abec749facff ("fs/proc/page.c: allow inspection of last section
>> and fix end detection")
> 
> Are you sure that that commit broke it?

Reverting the above commit also "fixes" kpageflags, otherwise
kpageflags_read() returns 0 in the following check:
	if (src >= max_dump_pfn * KPMSIZE)
		return 0;

> I recall that we would naturally run into the limit, because
> 
> count = min_t(size_t, count, (max_pfn * KPMSIZE) - src);

The function returns before we reach this line.

Thanks,
Georgi

> wouldn't really do what you would expect either. But you could 
> force-read beyond max_pfn, yes, because the count computation was just 
> weird.
> 
> 
> I think the real issue is not properly adjusting max_pfn in the first 
> place when we introduced memoruy hotplug on arm64

David Hildenbrand Sept. 27, 2021, 8:14 p.m. UTC | #9

On 27.09.21 22:00, Georgi Djakov wrote:
> On 9/27/2021 8:34 PM, David Hildenbrand wrote:
>> On 27.09.21 19:22, Georgi Djakov wrote:
>>> On 9/24/2021 1:54 AM, Chris Goldsworthy wrote:
>>>> From: Sudarshan Rajagopalan <[e-mail]>
>>>>
>>>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>>>> needs updating to reflect on new PFNs being hot added to system.
>>>>
>>>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>>>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>>>
>>> Thanks for the patch, Chris!
>>>
>>> With this patch, the data in /proc/kpageflags appears to be correct and
>>> memory tools like procrank work again on arm64 platforms.
>>>
>>> Tested-by: Georgi Djakov <[e-mail]>
>>>
>>> Maybe we should add fixes tag, as it has been broken since the following
>>> commit:
>>> Fixes: abec749facff ("fs/proc/page.c: allow inspection of last section
>>> and fix end detection")
>>
>> Are you sure that that commit broke it?
> 
> Reverting the above commit also "fixes" kpageflags, otherwise
> kpageflags_read() returns 0 in the following check:
> 	if (src >= max_dump_pfn * KPMSIZE)
> 		return 0;
> 
>> I recall that we would naturally run into the limit, because
>>
>> count = min_t(size_t, count, (max_pfn * KPMSIZE) - src);
> 
> The function returns before we reach this line.

That is the old code. I don't see how the behavior of the old code with 
wrong max_pfn was doing what it's supposed to do.

page_idle and page_owner also rely on max_pfn. The root issue is that 
max_pfn wasn't updated properly.

Georgi Djakov Sept. 27, 2021, 11:01 p.m. UTC | #10

On 9/27/2021 11:14 PM, David Hildenbrand wrote:
> On 27.09.21 22:00, Georgi Djakov wrote:
>> On 9/27/2021 8:34 PM, David Hildenbrand wrote:
>>> On 27.09.21 19:22, Georgi Djakov wrote:
>>>> On 9/24/2021 1:54 AM, Chris Goldsworthy wrote:
>>>>> From: Sudarshan Rajagopalan <[e-mail]>
>>>>>
>>>>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>>>>> needs updating to reflect on new PFNs being hot added to system.
>>>>>
>>>>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>>>>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>>>>
>>>> Thanks for the patch, Chris!
>>>>
>>>> With this patch, the data in /proc/kpageflags appears to be correct and
>>>> memory tools like procrank work again on arm64 platforms.
>>>>
>>>> Tested-by: Georgi Djakov <[e-mail]>
>>>>
>>>> Maybe we should add fixes tag, as it has been broken since the 
>>>> following
>>>> commit:
>>>> Fixes: abec749facff ("fs/proc/page.c: allow inspection of last section
>>>> and fix end detection")
>>>
>>> Are you sure that that commit broke it?
>>
>> Reverting the above commit also "fixes" kpageflags, otherwise
>> kpageflags_read() returns 0 in the following check:
>>     if (src >= max_dump_pfn * KPMSIZE)
>>         return 0;
>>
>>> I recall that we would naturally run into the limit, because
>>>
>>> count = min_t(size_t, count, (max_pfn * KPMSIZE) - src);
>>
>> The function returns before we reach this line.
> 
> That is the old code. I don't see how the behavior of the old code with 
> wrong max_pfn was doing what it's supposed to do.
> 
> page_idle and page_owner also rely on max_pfn. The root issue is that 
> max_pfn wasn't updated properly.

Ok, i agree that this is not really fixing abec749facff, as it only 
reveals the real issue, so then the fixes tag should be even further
back to:
Fixes: 4ab215061554 ("arm64: Add memory hotplug support")

Thanks,
Georgi

Georgi Djakov Sept. 27, 2021, 11:22 p.m. UTC | #11

On 9/27/2021 6:51 PM, David Hildenbrand wrote:
> On 24.09.21 00:54, Chris Goldsworthy wrote:
>> From: Sudarshan Rajagopalan <[e-mail]>
>>
>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>> needs updating to reflect on new PFNs being hot added to system.
>>
>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>> ---
>>   arch/arm64/mm/mmu.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index cfd9deb..fd85b51 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>       if (ret)
>>           __remove_pgd_mapping(swapper_pg_dir,
>>                        __phys_to_virt(start), size);
>> +    else {
>> +        max_pfn = PFN_UP(start + size);
>> +        max_low_pfn = max_pfn;
>> +    }
>> +
>>       return ret;
> 
> Note: didn't verify if updating max_low_pfn is correct here.

My understanding is that max_low_pfn defines the low/high memory
boundary and it should be also updated.

Thanks,
Georgi

Chris Goldsworthy Sept. 28, 2021, 6:12 a.m. UTC | #12

On Tue, Sep 28, 2021 at 02:22:59AM +0300, Georgi Djakov wrote:
> On 9/27/2021 6:51 PM, David Hildenbrand wrote:
> >On 24.09.21 00:54, Chris Goldsworthy wrote:
> >>From: Sudarshan Rajagopalan <[e-mail]>
> >>
> >>After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> >>needs updating to reflect on new PFNs being hot added to system.
> >>
> >>Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> >>Signed-off-by: Chris Goldsworthy <[e-mail]>
> >>---
> >>  arch/arm64/mm/mmu.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >>diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >>index cfd9deb..fd85b51 100644
> >>--- a/arch/arm64/mm/mmu.c
> >>+++ b/arch/arm64/mm/mmu.c
> >>@@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >>      if (ret)
> >>          __remove_pgd_mapping(swapper_pg_dir,
> >>                       __phys_to_virt(start), size);
> >>+    else {
> >>+        max_pfn = PFN_UP(start + size);
> >>+        max_low_pfn = max_pfn;
> >>+    }
> >>+
> >>      return ret;
> >
> >Note: didn't verify if updating max_low_pfn is correct here.
> 
> My understanding is that max_low_pfn defines the low/high memory
> boundary and it should be also updated.
> 
> Thanks,
> Georgi

To build more on Georgi's response, our assumption here after an offline
discussion is that max_low_pfn would not be equal to max_pfn only if there is
high memory - another assumption is that arm64 kernels will not need high memory
due to their large logical mappings. Under these two assumptions, the patch is
correct. Perhaps Catalin can ack or critique this, as he initially set max_pfn =
max_low_pfn in the first arm64 mm initialization code:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c1cc1552616d0f354d040823151e61634e7ad01f

David Hildenbrand Sept. 28, 2021, 7:33 a.m. UTC | #13

On 28.09.21 08:12, Chris Goldsworthy wrote:
> On Tue, Sep 28, 2021 at 02:22:59AM +0300, Georgi Djakov wrote:
>> On 9/27/2021 6:51 PM, David Hildenbrand wrote:
>>> On 24.09.21 00:54, Chris Goldsworthy wrote:
>>>> From: Sudarshan Rajagopalan <[e-mail]>
>>>>
>>>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>>>> needs updating to reflect on new PFNs being hot added to system.
>>>>
>>>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>>>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>>>> ---
>>>>    arch/arm64/mm/mmu.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>> index cfd9deb..fd85b51 100644
>>>> --- a/arch/arm64/mm/mmu.c
>>>> +++ b/arch/arm64/mm/mmu.c
>>>> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>>>        if (ret)
>>>>            __remove_pgd_mapping(swapper_pg_dir,
>>>>                         __phys_to_virt(start), size);
>>>> +    else {
>>>> +        max_pfn = PFN_UP(start + size);
>>>> +        max_low_pfn = max_pfn;
>>>> +    }
>>>> +
>>>>        return ret;
>>>
>>> Note: didn't verify if updating max_low_pfn is correct here.
>>
>> My understanding is that max_low_pfn defines the low/high memory
>> boundary and it should be also updated.
>>
>> Thanks,
>> Georgi
> 
> To build more on Georgi's response, our assumption here after an offline
> discussion is that max_low_pfn would not be equal to max_pfn only if there is
> high memory - another assumption is that arm64 kernels will not need high memory
> due to their large logical mappings. Under these two assumptions, the patch is
> correct. Perhaps Catalin can ack or critique this, as he initially set max_pfn =
> max_low_pfn in the first arm64 mm initialization code:

Makes sense to me, thanks.

Will Deacon Sept. 29, 2021, 10:10 a.m. UTC | #14

On Thu, Sep 23, 2021 at 03:54:48PM -0700, Chris Goldsworthy wrote:
> From: Sudarshan Rajagopalan <[e-mail]>
> 
> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> needs updating to reflect on new PFNs being hot added to system.
> 
> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> Signed-off-by: Chris Goldsworthy <[e-mail]>
> ---
>  arch/arm64/mm/mmu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index cfd9deb..fd85b51 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>  	if (ret)
>  		__remove_pgd_mapping(swapper_pg_dir,
>  				     __phys_to_virt(start), size);
> +	else {
> +		max_pfn = PFN_UP(start + size);
> +		max_low_pfn = max_pfn;
> +	}

We use 'max_pfn' as part of the argument to set_max_mapnr(). Does that need
updating as well?

Do we have sufficient locking to ensure nobody is looking at max_pfn or
max_low_pfn while we update them?

Will

David Hildenbrand Sept. 29, 2021, 10:29 a.m. UTC | #15

On 29.09.21 12:10, Will Deacon wrote:
> On Thu, Sep 23, 2021 at 03:54:48PM -0700, Chris Goldsworthy wrote:
>> From: Sudarshan Rajagopalan <[e-mail]>
>>
>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>> needs updating to reflect on new PFNs being hot added to system.
>>
>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>> ---
>>   arch/arm64/mm/mmu.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index cfd9deb..fd85b51 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>   	if (ret)
>>   		__remove_pgd_mapping(swapper_pg_dir,
>>   				     __phys_to_virt(start), size);
>> +	else {
>> +		max_pfn = PFN_UP(start + size);
>> +		max_low_pfn = max_pfn;
>> +	}
> 
> We use 'max_pfn' as part of the argument to set_max_mapnr(). Does that need
> updating as well?
> 
> Do we have sufficient locking to ensure nobody is looking at max_pfn or
> max_low_pfn while we update them?

Only the write side is protected by memory hotplug locking. The read 
side is lockless -- just like all of the other pfn_to_online_page() 
machinery.

> 
> Will
>

Will Deacon Sept. 29, 2021, 10:42 a.m. UTC | #16

On Wed, Sep 29, 2021 at 12:29:32PM +0200, David Hildenbrand wrote:
> On 29.09.21 12:10, Will Deacon wrote:
> > On Thu, Sep 23, 2021 at 03:54:48PM -0700, Chris Goldsworthy wrote:
> > > From: Sudarshan Rajagopalan <[e-mail]>
> > > 
> > > After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> > > needs updating to reflect on new PFNs being hot added to system.
> > > 
> > > Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> > > Signed-off-by: Chris Goldsworthy <[e-mail]>
> > > ---
> > >   arch/arm64/mm/mmu.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > > index cfd9deb..fd85b51 100644
> > > --- a/arch/arm64/mm/mmu.c
> > > +++ b/arch/arm64/mm/mmu.c
> > > @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
> > >   	if (ret)
> > >   		__remove_pgd_mapping(swapper_pg_dir,
> > >   				     __phys_to_virt(start), size);
> > > +	else {
> > > +		max_pfn = PFN_UP(start + size);
> > > +		max_low_pfn = max_pfn;
> > > +	}
> > 
> > We use 'max_pfn' as part of the argument to set_max_mapnr(). Does that need
> > updating as well?
> > 
> > Do we have sufficient locking to ensure nobody is looking at max_pfn or
> > max_low_pfn while we update them?
> 
> Only the write side is protected by memory hotplug locking. The read side is
> lockless -- just like all of the other pfn_to_online_page() machinery.

Hmm. So the readers can see one of the variables updated but the other one
stale?

Will

David Hildenbrand Sept. 29, 2021, 10:49 a.m. UTC | #17

On 29.09.21 12:42, Will Deacon wrote:
> On Wed, Sep 29, 2021 at 12:29:32PM +0200, David Hildenbrand wrote:
>> On 29.09.21 12:10, Will Deacon wrote:
>>> On Thu, Sep 23, 2021 at 03:54:48PM -0700, Chris Goldsworthy wrote:
>>>> From: Sudarshan Rajagopalan <[e-mail]>
>>>>
>>>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>>>> needs updating to reflect on new PFNs being hot added to system.
>>>>
>>>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>>>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>>>> ---
>>>>    arch/arm64/mm/mmu.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>> index cfd9deb..fd85b51 100644
>>>> --- a/arch/arm64/mm/mmu.c
>>>> +++ b/arch/arm64/mm/mmu.c
>>>> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>>>    	if (ret)
>>>>    		__remove_pgd_mapping(swapper_pg_dir,
>>>>    				     __phys_to_virt(start), size);
>>>> +	else {
>>>> +		max_pfn = PFN_UP(start + size);
>>>> +		max_low_pfn = max_pfn;
>>>> +	}
>>>
>>> We use 'max_pfn' as part of the argument to set_max_mapnr(). Does that need
>>> updating as well?
>>>
>>> Do we have sufficient locking to ensure nobody is looking at max_pfn or
>>> max_low_pfn while we update them?
>>
>> Only the write side is protected by memory hotplug locking. The read side is
>> lockless -- just like all of the other pfn_to_online_page() machinery.
> 
> Hmm. So the readers can see one of the variables updated but the other one
> stale?

Yes, just like it has been on x86-64 for a long time:

arch/x86/mm/init_64.c:update_end_of_memory_vars()

Not sure if anyone really cares about slightly delayed updates while 
memory is getting hotplugged. The users that I am aware of don't care.

Will Deacon Sept. 29, 2021, 11:03 a.m. UTC | #18

On Wed, Sep 29, 2021 at 12:49:58PM +0200, David Hildenbrand wrote:
> On 29.09.21 12:42, Will Deacon wrote:
> > On Wed, Sep 29, 2021 at 12:29:32PM +0200, David Hildenbrand wrote:
> > > On 29.09.21 12:10, Will Deacon wrote:
> > > > On Thu, Sep 23, 2021 at 03:54:48PM -0700, Chris Goldsworthy wrote:
> > > > > From: Sudarshan Rajagopalan <[e-mail]>
> > > > > 
> > > > > After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> > > > > needs updating to reflect on new PFNs being hot added to system.
> > > > > 
> > > > > Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> > > > > Signed-off-by: Chris Goldsworthy <[e-mail]>
> > > > > ---
> > > > >    arch/arm64/mm/mmu.c | 5 +++++
> > > > >    1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > > > > index cfd9deb..fd85b51 100644
> > > > > --- a/arch/arm64/mm/mmu.c
> > > > > +++ b/arch/arm64/mm/mmu.c
> > > > > @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
> > > > >    	if (ret)
> > > > >    		__remove_pgd_mapping(swapper_pg_dir,
> > > > >    				     __phys_to_virt(start), size);
> > > > > +	else {
> > > > > +		max_pfn = PFN_UP(start + size);
> > > > > +		max_low_pfn = max_pfn;
> > > > > +	}
> > > > 
> > > > We use 'max_pfn' as part of the argument to set_max_mapnr(). Does that need
> > > > updating as well?
> > > > 
> > > > Do we have sufficient locking to ensure nobody is looking at max_pfn or
> > > > max_low_pfn while we update them?
> > > 
> > > Only the write side is protected by memory hotplug locking. The read side is
> > > lockless -- just like all of the other pfn_to_online_page() machinery.
> > 
> > Hmm. So the readers can see one of the variables updated but the other one
> > stale?
> 
> Yes, just like it has been on x86-64 for a long time:
> 
> arch/x86/mm/init_64.c:update_end_of_memory_vars()
> 
> Not sure if anyone really cares about slightly delayed updates while memory
> is getting hotplugged. The users that I am aware of don't care.

Thanks, I'd missed that x86 also updates max_low_pfn. So at least we're not
worse off in that respect.

Looking at set_max_mapnr(), I'm wondering why we need to call that at all
on arm64 as 'max_mapnr' only seems to be used for nommu.

Will

David Hildenbrand Sept. 29, 2021, 12:09 p.m. UTC | #19

On 29.09.21 13:03, Will Deacon wrote:
> On Wed, Sep 29, 2021 at 12:49:58PM +0200, David Hildenbrand wrote:
>> On 29.09.21 12:42, Will Deacon wrote:
>>> On Wed, Sep 29, 2021 at 12:29:32PM +0200, David Hildenbrand wrote:
>>>> On 29.09.21 12:10, Will Deacon wrote:
>>>>> On Thu, Sep 23, 2021 at 03:54:48PM -0700, Chris Goldsworthy wrote:
>>>>>> From: Sudarshan Rajagopalan <[e-mail]>
>>>>>>
>>>>>> After new memory blocks have been hotplugged, max_pfn and max_low_pfn
>>>>>> needs updating to reflect on new PFNs being hot added to system.
>>>>>>
>>>>>> Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
>>>>>> Signed-off-by: Chris Goldsworthy <[e-mail]>
>>>>>> ---
>>>>>>     arch/arm64/mm/mmu.c | 5 +++++
>>>>>>     1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>>>> index cfd9deb..fd85b51 100644
>>>>>> --- a/arch/arm64/mm/mmu.c
>>>>>> +++ b/arch/arm64/mm/mmu.c
>>>>>> @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>>>>>     	if (ret)
>>>>>>     		__remove_pgd_mapping(swapper_pg_dir,
>>>>>>     				     __phys_to_virt(start), size);
>>>>>> +	else {
>>>>>> +		max_pfn = PFN_UP(start + size);
>>>>>> +		max_low_pfn = max_pfn;
>>>>>> +	}
>>>>>
>>>>> We use 'max_pfn' as part of the argument to set_max_mapnr(). Does that need
>>>>> updating as well?
>>>>>
>>>>> Do we have sufficient locking to ensure nobody is looking at max_pfn or
>>>>> max_low_pfn while we update them?
>>>>
>>>> Only the write side is protected by memory hotplug locking. The read side is
>>>> lockless -- just like all of the other pfn_to_online_page() machinery.
>>>
>>> Hmm. So the readers can see one of the variables updated but the other one
>>> stale?
>>
>> Yes, just like it has been on x86-64 for a long time:
>>
>> arch/x86/mm/init_64.c:update_end_of_memory_vars()
>>
>> Not sure if anyone really cares about slightly delayed updates while memory
>> is getting hotplugged. The users that I am aware of don't care.
> 
> Thanks, I'd missed that x86 also updates max_low_pfn. So at least we're not
> worse off in that respect.
> 
> Looking at set_max_mapnr(), I'm wondering why we need to call that at all
> on arm64 as 'max_mapnr' only seems to be used for nommu.

I think max_mapnr is only helpful without SPARSE, I can spot the most 
prominent consumer being simplistic pfn_valid() implementation.

MEMORY_HOTPLUG on arm64 implies SPARSE. ... and I recall that FLATMEM is 
no longer possible on arm64. So most probably the arm64 call of 
set_max_mapnr() can just be dropped.

Will Deacon Sept. 29, 2021, 12:51 p.m. UTC | #20

On Wed, Sep 29, 2021 at 02:09:35PM +0200, David Hildenbrand wrote:
> On 29.09.21 13:03, Will Deacon wrote:
> > On Wed, Sep 29, 2021 at 12:49:58PM +0200, David Hildenbrand wrote:
> > > On 29.09.21 12:42, Will Deacon wrote:
> > > > On Wed, Sep 29, 2021 at 12:29:32PM +0200, David Hildenbrand wrote:
> > > > > On 29.09.21 12:10, Will Deacon wrote:
> > > > > > On Thu, Sep 23, 2021 at 03:54:48PM -0700, Chris Goldsworthy wrote:
> > > > > > > From: Sudarshan Rajagopalan <[e-mail]>
> > > > > > > 
> > > > > > > After new memory blocks have been hotplugged, max_pfn and max_low_pfn
> > > > > > > needs updating to reflect on new PFNs being hot added to system.
> > > > > > > 
> > > > > > > Signed-off-by: Sudarshan Rajagopalan <[e-mail]>
> > > > > > > Signed-off-by: Chris Goldsworthy <[e-mail]>
> > > > > > > ---
> > > > > > >     arch/arm64/mm/mmu.c | 5 +++++
> > > > > > >     1 file changed, 5 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > > > > > > index cfd9deb..fd85b51 100644
> > > > > > > --- a/arch/arm64/mm/mmu.c
> > > > > > > +++ b/arch/arm64/mm/mmu.c
> > > > > > > @@ -1499,6 +1499,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
> > > > > > >     	if (ret)
> > > > > > >     		__remove_pgd_mapping(swapper_pg_dir,
> > > > > > >     				     __phys_to_virt(start), size);
> > > > > > > +	else {
> > > > > > > +		max_pfn = PFN_UP(start + size);
> > > > > > > +		max_low_pfn = max_pfn;
> > > > > > > +	}
> > > > > > 
> > > > > > We use 'max_pfn' as part of the argument to set_max_mapnr(). Does that need
> > > > > > updating as well?
> > > > > > 
> > > > > > Do we have sufficient locking to ensure nobody is looking at max_pfn or
> > > > > > max_low_pfn while we update them?
> > > > > 
> > > > > Only the write side is protected by memory hotplug locking. The read side is
> > > > > lockless -- just like all of the other pfn_to_online_page() machinery.
> > > > 
> > > > Hmm. So the readers can see one of the variables updated but the other one
> > > > stale?
> > > 
> > > Yes, just like it has been on x86-64 for a long time:
> > > 
> > > arch/x86/mm/init_64.c:update_end_of_memory_vars()
> > > 
> > > Not sure if anyone really cares about slightly delayed updates while memory
> > > is getting hotplugged. The users that I am aware of don't care.
> > 
> > Thanks, I'd missed that x86 also updates max_low_pfn. So at least we're not
> > worse off in that respect.
> > 
> > Looking at set_max_mapnr(), I'm wondering why we need to call that at all
> > on arm64 as 'max_mapnr' only seems to be used for nommu.
> 
> I think max_mapnr is only helpful without SPARSE, I can spot the most
> prominent consumer being simplistic pfn_valid() implementation.

Yeah, and that's only used #ifndef CONFIG_MMU (there's a #error otherwise at
the top of the file).

> MEMORY_HOTPLUG on arm64 implies SPARSE. ... and I recall that FLATMEM is no
> longer possible on arm64. So most probably the arm64 call of set_max_mapnr()
> can just be dropped.

I'll do that and see if anything catches fire.

Will

Patch

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index cfd9deb..fd85b51 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1499,6 +1499,11 @@  int arch_add_memory(int nid, u64 start, u64 size,
 	if (ret)
 		__remove_pgd_mapping(swapper_pg_dir,
 				     __phys_to_virt(start), size);
+	else {
+		max_pfn = PFN_UP(start + size);
+		max_low_pfn = max_pfn;
+	}
+
 	return ret;
 }