[PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Anshuman Khandual]

In add_memory_resource() the memory range to be hot added first gets into
the memblock via memblock_add() before arch_add_memory() is called on it.
Reverse sequence should be followed during memory hot removal which already
is being followed in add_memory_resource() error path. This now ensures
required re-order between memblock_[free|remove]() and arch_remove_memory()
during memory hot-remove.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Michal Hocko <mhocko@suse.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Original patch https://lkml.org/lkml/2019/9/3/327

Memory hot remove now works on arm64 without this because a recent commit
60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").

David mentioned that re-ordering should still make sense for consistency
purpose (removing stuff in the reverse order they were added). This patch
is now detached from arm64 hot-remove series.

https://lkml.org/lkml/2019/9/3/326

 mm/memory_hotplug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c73f09913165..355c466e0621 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1770,13 +1770,13 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
 
 	/* remove memmap entry */
 	firmware_map_remove(start, start + size, "System RAM");
-	memblock_free(start, size);
-	memblock_remove(start, size);
 
 	/* remove memory block devices before removing memory */
 	remove_memory_block_devices(start, size);
 
 	arch_remove_memory(nid, start, size, NULL);
+	memblock_free(start, size);
+	memblock_remove(start, size);
 	__release_memory_resource(start, size);
 
 	try_offline_node(nid);
-- 
2.20.1

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Mike Rapoport]

On Mon, Sep 16, 2019 at 11:17:37AM +0530, Anshuman Khandual wrote:
> In add_memory_resource() the memory range to be hot added first gets into
> the memblock via memblock_add() before arch_add_memory() is called on it.
> Reverse sequence should be followed during memory hot removal which already
> is being followed in add_memory_resource() error path. This now ensures
> required re-order between memblock_[free|remove]() and arch_remove_memory()
> during memory hot-remove.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Original patch https://lkml.org/lkml/2019/9/3/327
> 
> Memory hot remove now works on arm64 without this because a recent commit
> 60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").
> 
> David mentioned that re-ordering should still make sense for consistency
> purpose (removing stuff in the reverse order they were added). This patch
> is now detached from arm64 hot-remove series.
> 
> https://lkml.org/lkml/2019/9/3/326
> 
>  mm/memory_hotplug.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..355c466e0621 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1770,13 +1770,13 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
> 
>  	/* remove memmap entry */
>  	firmware_map_remove(start, start + size, "System RAM");
> -	memblock_free(start, size);
> -	memblock_remove(start, size);
> 
>  	/* remove memory block devices before removing memory */
>  	remove_memory_block_devices(start, size);
> 
>  	arch_remove_memory(nid, start, size, NULL);
> +	memblock_free(start, size);

I don't see memblock_reserve() anywhere in memory_hotplug.c, so the
memblock_free() call here seems superfluous. I think it can be simply
dropped.

> +	memblock_remove(start, size);
>  	__release_memory_resource(start, size);
> 
>  	try_offline_node(nid);
> -- 
> 2.20.1
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Anshuman Khandual]

On 09/16/2019 12:06 PM, Mike Rapoport wrote:
> On Mon, Sep 16, 2019 at 11:17:37AM +0530, Anshuman Khandual wrote:
>> In add_memory_resource() the memory range to be hot added first gets into
>> the memblock via memblock_add() before arch_add_memory() is called on it.
>> Reverse sequence should be followed during memory hot removal which already
>> is being followed in add_memory_resource() error path. This now ensures
>> required re-order between memblock_[free|remove]() and arch_remove_memory()
>> during memory hot-remove.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> Original patch https://lkml.org/lkml/2019/9/3/327
>>
>> Memory hot remove now works on arm64 without this because a recent commit
>> 60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").
>>
>> David mentioned that re-ordering should still make sense for consistency
>> purpose (removing stuff in the reverse order they were added). This patch
>> is now detached from arm64 hot-remove series.
>>
>> https://lkml.org/lkml/2019/9/3/326
>>
>>  mm/memory_hotplug.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index c73f09913165..355c466e0621 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1770,13 +1770,13 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
>>
>>  	/* remove memmap entry */
>>  	firmware_map_remove(start, start + size, "System RAM");
>> -	memblock_free(start, size);
>> -	memblock_remove(start, size);
>>
>>  	/* remove memory block devices before removing memory */
>>  	remove_memory_block_devices(start, size);
>>
>>  	arch_remove_memory(nid, start, size, NULL);
>> +	memblock_free(start, size);
> 
> I don't see memblock_reserve() anywhere in memory_hotplug.c, so the
> memblock_free() call here seems superfluous. I think it can be simply
> dropped.

I had observed that previously but was not sure whether or not there are
still scenarios where it might be true. Error path in add_memory_resource()
even just calls memblock_remove() not memblock_free(). Unless there is any
objection, can just drop it.

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Anshuman Khandual]

On 09/16/2019 02:20 PM, Anshuman Khandual wrote:
> 
> 
> On 09/16/2019 12:06 PM, Mike Rapoport wrote:
>> On Mon, Sep 16, 2019 at 11:17:37AM +0530, Anshuman Khandual wrote:
>>> In add_memory_resource() the memory range to be hot added first gets into
>>> the memblock via memblock_add() before arch_add_memory() is called on it.
>>> Reverse sequence should be followed during memory hot removal which already
>>> is being followed in add_memory_resource() error path. This now ensures
>>> required re-order between memblock_[free|remove]() and arch_remove_memory()
>>> during memory hot-remove.
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Oscar Salvador <osalvador@suse.de>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> Original patch https://lkml.org/lkml/2019/9/3/327
>>>
>>> Memory hot remove now works on arm64 without this because a recent commit
>>> 60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").
>>>
>>> David mentioned that re-ordering should still make sense for consistency
>>> purpose (removing stuff in the reverse order they were added). This patch
>>> is now detached from arm64 hot-remove series.
>>>
>>> https://lkml.org/lkml/2019/9/3/326
>>>
>>>  mm/memory_hotplug.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index c73f09913165..355c466e0621 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1770,13 +1770,13 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
>>>
>>>  	/* remove memmap entry */
>>>  	firmware_map_remove(start, start + size, "System RAM");
>>> -	memblock_free(start, size);
>>> -	memblock_remove(start, size);
>>>
>>>  	/* remove memory block devices before removing memory */
>>>  	remove_memory_block_devices(start, size);
>>>
>>>  	arch_remove_memory(nid, start, size, NULL);
>>> +	memblock_free(start, size);
>>
>> I don't see memblock_reserve() anywhere in memory_hotplug.c, so the
>> memblock_free() call here seems superfluous. I think it can be simply
>> dropped.
> 
> I had observed that previously but was not sure whether or not there are
> still scenarios where it might be true. Error path in add_memory_resource()
> even just calls memblock_remove() not memblock_free(). Unless there is any
> objection, can just drop it.

Hello Mike,

Looks like we might need memblock_free() here as well. As you mentioned
before there might not be any memblock_reserve() in mm/memory_hotplug.c
but that does not guarantee that there could not have been a previous
memblock_reserve() or memblock_alloc_XXX() allocation which came from
the current hot remove range.

memblock_free() followed by memblock_remove() on the entire hot-remove
range ensures that memblock.memory and memblock.reserve are in sync and
the entire range is guaranteed to be removed from both the memory types.
Or am I missing something here.

- Anshuman

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Anshuman Khandual]

On 09/16/2019 11:17 AM, Anshuman Khandual wrote:
> In add_memory_resource() the memory range to be hot added first gets into
> the memblock via memblock_add() before arch_add_memory() is called on it.
> Reverse sequence should be followed during memory hot removal which already
> is being followed in add_memory_resource() error path. This now ensures
> required re-order between memblock_[free|remove]() and arch_remove_memory()
> during memory hot-remove.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Original patch https://lkml.org/lkml/2019/9/3/327
> 
> Memory hot remove now works on arm64 without this because a recent commit
> 60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").
> 
> David mentioned that re-ordering should still make sense for consistency
> purpose (removing stuff in the reverse order they were added). This patch
> is now detached from arm64 hot-remove series.
> 
> https://lkml.org/lkml/2019/9/3/326
> 
>  mm/memory_hotplug.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..355c466e0621 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1770,13 +1770,13 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
>  
>  	/* remove memmap entry */
>  	firmware_map_remove(start, start + size, "System RAM");
> -	memblock_free(start, size);
> -	memblock_remove(start, size);
>  
>  	/* remove memory block devices before removing memory */
>  	remove_memory_block_devices(start, size);
>  
>  	arch_remove_memory(nid, start, size, NULL);
> +	memblock_free(start, size);
> +	memblock_remove(start, size);
>  	__release_memory_resource(start, size);
>  
>  	try_offline_node(nid);
> 

Hello Andrew,

Any feedbacks on this, does it look okay ?

- Anshuman

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Andrew Morton]

On Mon, 23 Sep 2019 11:16:38 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:

> 
> 
> On 09/16/2019 11:17 AM, Anshuman Khandual wrote:
> > In add_memory_resource() the memory range to be hot added first gets into
> > the memblock via memblock_add() before arch_add_memory() is called on it.
> > Reverse sequence should be followed during memory hot removal which already
> > is being followed in add_memory_resource() error path. This now ensures
> > required re-order between memblock_[free|remove]() and arch_remove_memory()
> > during memory hot-remove.
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Oscar Salvador <osalvador@suse.de>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > ---
> > Original patch https://lkml.org/lkml/2019/9/3/327
> > 
> > Memory hot remove now works on arm64 without this because a recent commit
> > 60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").
> > 
> > David mentioned that re-ordering should still make sense for consistency
> > purpose (removing stuff in the reverse order they were added). This patch
> > is now detached from arm64 hot-remove series.
> > 
> > https://lkml.org/lkml/2019/9/3/326
>
> ...
>
> Hello Andrew,
> 
> Any feedbacks on this, does it look okay ?
> 

Well.  I'd parked this for 5.4-rc1 processing because it looked like a
cleanup.

But waaaay down below the ^---$ line I see "Memory hot remove now works
on arm64".  Am I correct in believing that 60bb462fc7ad broke arm64 mem
hot remove?  And that this patch fixes a serious regression?  If so,
that should have been right there in the patch title and changelog!

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Anshuman Khandual]

On 09/25/2019 08:43 AM, Andrew Morton wrote:
> On Mon, 23 Sep 2019 11:16:38 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
>>
>>
>> On 09/16/2019 11:17 AM, Anshuman Khandual wrote:
>>> In add_memory_resource() the memory range to be hot added first gets into
>>> the memblock via memblock_add() before arch_add_memory() is called on it.
>>> Reverse sequence should be followed during memory hot removal which already
>>> is being followed in add_memory_resource() error path. This now ensures
>>> required re-order between memblock_[free|remove]() and arch_remove_memory()
>>> during memory hot-remove.
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Oscar Salvador <osalvador@suse.de>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> Original patch https://lkml.org/lkml/2019/9/3/327
>>>
>>> Memory hot remove now works on arm64 without this because a recent commit
>>> 60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").
>>>
>>> David mentioned that re-ordering should still make sense for consistency
>>> purpose (removing stuff in the reverse order they were added). This patch
>>> is now detached from arm64 hot-remove series.
>>>
>>> https://lkml.org/lkml/2019/9/3/326
>>
>> ...
>>
>> Hello Andrew,
>>
>> Any feedbacks on this, does it look okay ?
>>
> 
> Well.  I'd parked this for 5.4-rc1 processing because it looked like a
> cleanup.

This does not fix a serious problem. It just removes an inconsistency while
freeing resources during memory hot remove which for now does not pose a
real problem.

> 
> But waaaay down below the ^---$ line I see "Memory hot remove now works
> on arm64".  Am I correct in believing that 60bb462fc7ad broke arm64 mem
> hot remove?  And that this patch fixes a serious regression?  If so,

No. [Proposed] arm64 memory hot remove series does not anymore depend on
this particular patch because 60bb462fc7ad has already solved the problem.

> that should have been right there in the patch title and changelog!

V2 (https://patchwork.kernel.org/patch/11159939/) for this patch makes it
very clear in it's commit message.

- Anshuman

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Michal Hocko]

On Mon 16-09-19 11:17:37, Anshuman Khandual wrote:
> In add_memory_resource() the memory range to be hot added first gets into
> the memblock via memblock_add() before arch_add_memory() is called on it.
> Reverse sequence should be followed during memory hot removal which already
> is being followed in add_memory_resource() error path. This now ensures
> required re-order between memblock_[free|remove]() and arch_remove_memory()
> during memory hot-remove.

This changelog is not really easy to follow. First of all please make
sure to explain whether there is any actual problem to solve or this is
an aesthetic matter. Please think of people reading this changelog in
few years and scratching their heads what you were thinking back then...

> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Original patch https://lkml.org/lkml/2019/9/3/327
> 
> Memory hot remove now works on arm64 without this because a recent commit
> 60bb462fc7ad ("drivers/base/node.c: simplify unregister_memory_block_under_nodes()").
> 
> David mentioned that re-ordering should still make sense for consistency
> purpose (removing stuff in the reverse order they were added). This patch
> is now detached from arm64 hot-remove series.
> 
> https://lkml.org/lkml/2019/9/3/326
> 
>  mm/memory_hotplug.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c73f09913165..355c466e0621 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1770,13 +1770,13 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
>  
>  	/* remove memmap entry */
>  	firmware_map_remove(start, start + size, "System RAM");
> -	memblock_free(start, size);
> -	memblock_remove(start, size);
>  
>  	/* remove memory block devices before removing memory */
>  	remove_memory_block_devices(start, size);
>  
>  	arch_remove_memory(nid, start, size, NULL);
> +	memblock_free(start, size);
> +	memblock_remove(start, size);
>  	__release_memory_resource(start, size);
>  
>  	try_offline_node(nid);
> -- 
> 2.20.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[David Hildenbrand]

On 23.09.19 12:52, Michal Hocko wrote:
> On Mon 16-09-19 11:17:37, Anshuman Khandual wrote:
>> In add_memory_resource() the memory range to be hot added first gets into
>> the memblock via memblock_add() before arch_add_memory() is called on it.
>> Reverse sequence should be followed during memory hot removal which already
>> is being followed in add_memory_resource() error path. This now ensures
>> required re-order between memblock_[free|remove]() and arch_remove_memory()
>> during memory hot-remove.
> 
> This changelog is not really easy to follow. First of all please make
> sure to explain whether there is any actual problem to solve or this is
> an aesthetic matter. Please think of people reading this changelog in
> few years and scratching their heads what you were thinking back then...
> 

I think it would make sense to just draft the current call sequence in
the add and the removal path (instead of describing it) - then it
becomes obvious why this is a cosmetic change.

-- 

Thanks,

David / dhildenb

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Anshuman Khandual]

On 09/23/2019 04:24 PM, David Hildenbrand wrote:
> On 23.09.19 12:52, Michal Hocko wrote:
>> On Mon 16-09-19 11:17:37, Anshuman Khandual wrote:
>>> In add_memory_resource() the memory range to be hot added first gets into
>>> the memblock via memblock_add() before arch_add_memory() is called on it.
>>> Reverse sequence should be followed during memory hot removal which already
>>> is being followed in add_memory_resource() error path. This now ensures
>>> required re-order between memblock_[free|remove]() and arch_remove_memory()
>>> during memory hot-remove.
>>
>> This changelog is not really easy to follow. First of all please make
>> sure to explain whether there is any actual problem to solve or this is
>> an aesthetic matter. Please think of people reading this changelog in
>> few years and scratching their heads what you were thinking back then...
>>
> 
> I think it would make sense to just draft the current call sequence in
> the add and the removal path (instead of describing it) - then it
> becomes obvious why this is a cosmetic change.

Does this look okay ?

mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

Currently during memory hot add procedure, memory gets into memblock before
calling arch_add_memory() which creates it's linear mapping.

add_memory_resource() {
        ..................
        memblock_add_node()
        ..................
        arch_add_memory()
        ..................
}

But during memory hot remove procedure, removal from memblock happens first
before it's linear mapping gets teared down with arch_remove_memory() which
is not coherent. Resource removal should happen in reverse order as they
were added.

try_remove_memory() {
        ..................
        memblock_free()
        memblock_remove()
        ..................
        arch_remove_memory()
        ..................
}

This changes the sequence of resource removal including memblock and linear
mapping tear down during memory hot remove which will now be the reverse
order in which they were added during memory hot add. The changed removal
order looks like the following.

try_remove_memory() {
        ..................
        arch_remove_memory()
        ..................
        memblock_free()
        memblock_remove()
        ..................
}

Re: [PATCH] mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()

[Michal Hocko]

On Tue 24-09-19 09:42:31, Anshuman Khandual wrote:
> 
> 
> On 09/23/2019 04:24 PM, David Hildenbrand wrote:
> > On 23.09.19 12:52, Michal Hocko wrote:
> >> On Mon 16-09-19 11:17:37, Anshuman Khandual wrote:
> >>> In add_memory_resource() the memory range to be hot added first gets into
> >>> the memblock via memblock_add() before arch_add_memory() is called on it.
> >>> Reverse sequence should be followed during memory hot removal which already
> >>> is being followed in add_memory_resource() error path. This now ensures
> >>> required re-order between memblock_[free|remove]() and arch_remove_memory()
> >>> during memory hot-remove.
> >>
> >> This changelog is not really easy to follow. First of all please make
> >> sure to explain whether there is any actual problem to solve or this is
> >> an aesthetic matter. Please think of people reading this changelog in
> >> few years and scratching their heads what you were thinking back then...
> >>
> > 
> > I think it would make sense to just draft the current call sequence in
> > the add and the removal path (instead of describing it) - then it
> > becomes obvious why this is a cosmetic change.
> 
> Does this look okay ?
> 
> mm/hotplug: Reorder memblock_[free|remove]() calls in try_remove_memory()
> 
> Currently during memory hot add procedure, memory gets into memblock before
> calling arch_add_memory() which creates it's linear mapping.
> 
> add_memory_resource() {
>         ..................
>         memblock_add_node()
>         ..................
>         arch_add_memory()
>         ..................
> }
> 
> But during memory hot remove procedure, removal from memblock happens first
> before it's linear mapping gets teared down with arch_remove_memory() which
> is not coherent. Resource removal should happen in reverse order as they
> were added.
> 
> try_remove_memory() {
>         ..................
>         memblock_free()
>         memblock_remove()
>         ..................
>         arch_remove_memory()
>         ..................
> }
> 
> This changes the sequence of resource removal including memblock and linear
> mapping tear down during memory hot remove which will now be the reverse
> order in which they were added during memory hot add. The changed removal
> order looks like the following.
> 
> try_remove_memory() {
>         ..................
>         arch_remove_memory()
>         ..................
>         memblock_free()
>         memblock_remove()
>         ..................
> }

OK, this is better. I would just a note that the inconsistency doesn't
pose any problem now but if somebody makes any assumptions about linear
mappings then it could get subtly broken like your example for arm64
which has found a different solution in the meantime.

-- 
Michal Hocko
SUSE Labs