Re: [PATCH] mm/memblock: remove empty dummy entry

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

On Thu, Apr 04, 2024 at 01:13:41PM +0300, Mike Rapoport wrote:
>On Wed, Apr 03, 2024 at 09:10:45AM +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>
>> ---
>>  mm/memblock.c                            | 18 +++---------------
>>  tools/testing/memblock/tests/basic_api.c |  8 ++++----
>>  tools/testing/memblock/tests/common.c    |  4 ++--
>>  3 files changed, 9 insertions(+), 21 deletions(-)
>> 
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index d09136e040d3..100d3b7ab499 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -114,12 +114,12 @@ 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.cnt		= 0,
>
>No need to set memory.cnt to 0, this line can be just removed
>
>>  	.memory.max		= INIT_MEMBLOCK_MEMORY_REGIONS,
>>  	.memory.name		= "memory",
>>  
>>  	.reserved.regions	= memblock_reserved_init_regions,
>> -	.reserved.cnt		= 1,	/* empty dummy entry */
>> +	.reserved.cnt		= 0,
>
>Ditto.
>
>>  	.reserved.max		= INIT_MEMBLOCK_RESERVED_REGIONS,
>>  	.reserved.name		= "reserved",
>>  
>> @@ -130,7 +130,7 @@ 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 */
>> +	.cnt			= 0,
>
>Ditto.
>
>>  	.max			= INIT_PHYSMEM_REGIONS,
>>  	.name			= "physmem",
>>  };
>> @@ -356,7 +356,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;
>>  		type->regions[0].base = 0;
>>  		type->regions[0].size = 0;
>>  		type->regions[0].flags = 0;
>> @@ -598,17 +597,6 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>>  	if (!size)
>>  		return 0;
>>  
>> -	/* special case for empty array */
>> -	if (type->regions[0].size == 0) {
>> -		WARN_ON(type->cnt != 1 || type->total_size);
>> -		type->regions[0].base = base;
>> -		type->regions[0].size = size;
>> -		type->regions[0].flags = flags;
>> -		memblock_set_region_node(&type->regions[0], nid);
>> -		type->total_size = size;
>> -		return 0;
>> -	}
>
>I'd keep the special case for empty array, just update it for new semantics
>of type->cnt.
>
>> -
>>  	/*
>>  	 * The worst case is when new range overlaps all existing regions,
>>  	 * then we'll need type->cnt + 1 empty regions in @type. So if
>> diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c
>> index 57bf2688edfd..f317fe691fc4 100644
>> --- a/tools/testing/memblock/tests/basic_api.c
>> +++ b/tools/testing/memblock/tests/basic_api.c
>> @@ -15,12 +15,12 @@ static int memblock_initialization_check(void)
>>  	PREFIX_PUSH();
>>  
>>  	ASSERT_NE(memblock.memory.regions, NULL);
>> -	ASSERT_EQ(memblock.memory.cnt, 1);
>> +	ASSERT_EQ(memblock.memory.cnt, 0);
>>  	ASSERT_EQ(memblock.memory.max, EXPECTED_MEMBLOCK_REGIONS);
>>  	ASSERT_EQ(strcmp(memblock.memory.name, "memory"), 0);
>>  
>>  	ASSERT_NE(memblock.reserved.regions, NULL);
>> -	ASSERT_EQ(memblock.reserved.cnt, 1);
>> +	ASSERT_EQ(memblock.reserved.cnt, 0);
>>  	ASSERT_EQ(memblock.memory.max, EXPECTED_MEMBLOCK_REGIONS);
>>  	ASSERT_EQ(strcmp(memblock.reserved.name, "reserved"), 0);
>>  
>> @@ -1295,7 +1295,7 @@ static int memblock_remove_only_region_check(void)
>>  	ASSERT_EQ(rgn->base, 0);
>>  	ASSERT_EQ(rgn->size, 0);
>>  
>> -	ASSERT_EQ(memblock.memory.cnt, 1);
>> +	ASSERT_EQ(memblock.memory.cnt, 0);
>>  	ASSERT_EQ(memblock.memory.total_size, 0);
>>  
>>  	test_pass_pop();
>> @@ -1723,7 +1723,7 @@ static int memblock_free_only_region_check(void)
>>  	ASSERT_EQ(rgn->base, 0);
>>  	ASSERT_EQ(rgn->size, 0);
>>  
>> -	ASSERT_EQ(memblock.reserved.cnt, 1);
>> +	ASSERT_EQ(memblock.reserved.cnt, 0);
>>  	ASSERT_EQ(memblock.reserved.total_size, 0);
>>  
>>  	test_pass_pop();
>> diff --git a/tools/testing/memblock/tests/common.c b/tools/testing/memblock/tests/common.c
>> index f43b6f414983..4fa94b281280 100644
>> --- a/tools/testing/memblock/tests/common.c
>> +++ b/tools/testing/memblock/tests/common.c
>> @@ -40,13 +40,13 @@ void reset_memblock_regions(void)
>>  {
>>  	memset(memblock.memory.regions, 0,
>>  	       memblock.memory.cnt * sizeof(struct memblock_region));
>> -	memblock.memory.cnt	= 1;
>> +	memblock.memory.cnt	= 0;
>
>Here as well, no need to initialize to 0.
>

Oops, I found maybe we should keep this. Since reset_memblock_regions is
called on each new round test, we should clear it.

>>  	memblock.memory.max	= INIT_MEMBLOCK_REGIONS;
>>  	memblock.memory.total_size = 0;
>>  
>>  	memset(memblock.reserved.regions, 0,
>>  	       memblock.reserved.cnt * sizeof(struct memblock_region));
>> -	memblock.reserved.cnt	= 1;
>> +	memblock.reserved.cnt	= 0;
>>  	memblock.reserved.max	= INIT_MEMBLOCK_RESERVED_REGIONS;
>>  	memblock.reserved.total_size = 0;
>>  }
>> -- 
>> 2.34.1
>> 
>
>-- 
>Sincerely yours,
>Mike.

-- 
Wei Yang
Help you, Help me