Re: [Patch v3] mm/memblock: remove empty dummy entry

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Apr 08, 2024 at 08:44:31AM +0300, Mike Rapoport wrote:
>On Fri, Apr 05, 2024 at 01:58:21AM +0000, Wei Yang wrote:
>> The dummy entry is introduced in the initial implementation of lmb in
>> commit 7c8c6b9776fb ("powerpc: Merge lmb.c and make MM initialization
>> use it.").
>> 
>> As the comment says the empty dummy entry is to simplify the code.
>> 
>> 	/* Create a dummy zero size LMB which will get coalesced away later.
>>          * This simplifies the lmb_add() code below...
>>          */
>> 
>> While current code is reimplemented by Tejun in commit 784656f9c680
>> ("memblock: Reimplement memblock_add_region()"). This empty dummy entry
>> seems not benefit the code any more.
>> 
>> Let's remove it.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Paul Mackerras <paulus@ozlabs.org>
>> CC: Tejun Heo <tj@kernel.org>
>> CC: Mike Rapoport <rppt@kernel.org>
>> 
>> ---
>> v2: remove cnt initialization to 0 and keep special case for empty array
>> v3: reset cnt to 0 in memblock test
>> ---
>>  mm/memblock.c                            | 7 ++-----
>>  tools/testing/memblock/tests/basic_api.c | 8 ++++----
>>  tools/testing/memblock/tests/common.c    | 4 ++--
>>  3 files changed, 8 insertions(+), 11 deletions(-)
>> 
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index d09136e040d3..98d25689cf10 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -114,12 +114,10 @@ static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS
>>  
>>  struct memblock memblock __initdata_memblock = {
>>  	.memory.regions		= memblock_memory_init_regions,
>> -	.memory.cnt		= 1,	/* empty dummy entry */
>>  	.memory.max		= INIT_MEMBLOCK_MEMORY_REGIONS,
>>  	.memory.name		= "memory",
>>  
>>  	.reserved.regions	= memblock_reserved_init_regions,
>> -	.reserved.cnt		= 1,	/* empty dummy entry */
>>  	.reserved.max		= INIT_MEMBLOCK_RESERVED_REGIONS,
>>  	.reserved.name		= "reserved",
>>  
>> @@ -130,7 +128,6 @@ struct memblock memblock __initdata_memblock = {
>>  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
>>  struct memblock_type physmem = {
>>  	.regions		= memblock_physmem_init_regions,
>> -	.cnt			= 1,	/* empty dummy entry */
>>  	.max			= INIT_PHYSMEM_REGIONS,
>>  	.name			= "physmem",
>>  };
>> @@ -356,7 +353,6 @@ static void __init_memblock memblock_remove_region(struct memblock_type *type, u
>>  	/* Special case for empty arrays */
>>  	if (type->cnt == 0) {
>>  		WARN_ON(type->total_size != 0);
>> -		type->cnt = 1;
>
>Sorry if I wasn't clear, I meant to keep special case only in
>memblock_add_range. Here I think
>
>	WARN_ON(type->cnt == 0 && type->total_size != 0);
>
>should be enough.
>

You mean change like this?

diff --git a/mm/memblock.c b/mm/memblock.c
index d09136e040d3..b75b835f99d1 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -352,16 +352,7 @@ static void __init_memblock memblock_remove_region(struct memblock_type *type, u
 	memmove(&type->regions[r], &type->regions[r + 1],
 		(type->cnt - (r + 1)) * sizeof(type->regions[r]));
 	type->cnt--;
-
-	/* Special case for empty arrays */
-	if (type->cnt == 0) {
-		WARN_ON(type->total_size != 0);
-		type->cnt = 1;
-		type->regions[0].base = 0;
-		type->regions[0].size = 0;
-		type->regions[0].flags = 0;
-		memblock_set_region_node(&type->regions[0], MAX_NUMNODES);
-	}
+	WARN_ON(type->cnt == 0 && type->total_size != 0);
 }
 
 #ifndef CONFIG_ARCH_KEEP_MEMBLOCK


This doesn't work, since on removing the last region, memmove would take no
effect and left regions[0].base .size .flags not changed.

So we need to keep the special handling here.