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

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

[Wei Yang]

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;
 		type->regions[0].base = 0;
 		type->regions[0].size = 0;
 		type->regions[0].flags = 0;
@@ -600,12 +596,13 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
 
 	/* special case for empty array */
 	if (type->regions[0].size == 0) {
-		WARN_ON(type->cnt != 1 || type->total_size);
+		WARN_ON(type->cnt != 0 || 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;
+		type->cnt = 1;
 		return 0;
 	}
 
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..c2c569f12178 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;
 	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

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

[Mike Rapoport]

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.

>  		type->regions[0].base = 0;
>  		type->regions[0].size = 0;
>  		type->regions[0].flags = 0;
> @@ -600,12 +596,13 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>  
>  	/* special case for empty array */
>  	if (type->regions[0].size == 0) {
> -		WARN_ON(type->cnt != 1 || type->total_size);
> +		WARN_ON(type->cnt != 0 || 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;
> +		type->cnt = 1;
>  		return 0;
>  	}
>  
> 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..c2c569f12178 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;
>  	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.

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

[Wei Yang]

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.

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

[Mike Rapoport]

From: Mike Rapoport (IBM) <rppt@kernel.org>

On Fri, 05 Apr 2024 01:58:21 +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...
>          */
> 
> [...]

Applied to for-next branch of memblock.git tree, thanks!

[1/1] mm/memblock: remove empty dummy entry
      commit: e5d1fdecfaf8470bad3874d00c7921e3883495b7

tree: https://git.kernel.org/pub/scm/linux/kernel/git/rppt/memblock
branch: for-next

--
Sincerely yours,
Mike.

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

[Wei Yang]

On Thu, Jun 20, 2024 at 02:58:17PM -0700, Guenter Roeck wrote:
>Hi,
>
>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>
>> 
>
>With this patch in linux-next, all microblaze qemu images fail to boot. Reverting it
>fixes the problem.
>
>Bisect log is attached for reference.
>

Thanks for the reporting.

Would you mind running the test to take a look?

It is in tools/testing/memblock. Simply make and ./main.

>Guenter
>
>---
># bad: [2102cb0d050d34d50b9642a3a50861787527e922] Add linux-next specific files for 20240619
># good: [6ba59ff4227927d3a8530fc2973b80e94b54d58f] Linux 6.10-rc4
>git bisect start 'HEAD' 'v6.10-rc4'
># good: [a8fa5261ec87d5aafd3211548d93008d5739457d] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
>git bisect good a8fa5261ec87d5aafd3211548d93008d5739457d
># good: [ee551f4db89753511a399b808db75654facec7c8] Merge branch 'for-linux-next' of https://gitlab.freedesktop.org/drm/i915/kernel
>git bisect good ee551f4db89753511a399b808db75654facec7c8
># good: [ec3557f4b791d72d93bfb69702d441d2c9f8cd0d] Merge branch 'next' of git://git.kernel.org/pub/scm/virt/kvm/kvm.git
>git bisect good ec3557f4b791d72d93bfb69702d441d2c9f8cd0d
># good: [48d51b3acbb237074014d498d76ea6b6ce5aed69] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git
>git bisect good 48d51b3acbb237074014d498d76ea6b6ce5aed69
># good: [59972b98583cd97febf9ecc576a706a7c5046278] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
>git bisect good 59972b98583cd97febf9ecc576a706a7c5046278
># good: [49aa15d761ce8976bb131f06886e89bd10cdb9fd] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching
>git bisect good 49aa15d761ce8976bb131f06886e89bd10cdb9fd
># bad: [129b9b07cd69885a804319c3fac82ef79e012e07] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/rppt/memblock.git
>git bisect bad 129b9b07cd69885a804319c3fac82ef79e012e07
># good: [344db92cbecc8da1f58d559926c61ceb72e72a03] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git
>git bisect good 344db92cbecc8da1f58d559926c61ceb72e72a03
># bad: [ce8ebb95439459f7e24b02c6943e278f46d2d328] mm/mm_init.c: get the highest zone directly
>git bisect bad ce8ebb95439459f7e24b02c6943e278f46d2d328
># bad: [1a879671bdfd14698a839f30de8e6d76e1e858fd] memblock tests: add memblock_overlaps_region_checks
>git bisect bad 1a879671bdfd14698a839f30de8e6d76e1e858fd
># bad: [3d3165193776ddacf59f101f0fa05cfab9f1a9ba] memblock tests: add memblock_reserve_all_locations_check()
>git bisect bad 3d3165193776ddacf59f101f0fa05cfab9f1a9ba
># bad: [721f4a6526daafca15634f30c9865e880da3e1d1] mm/memblock: remove empty dummy entry
>git bisect bad 721f4a6526daafca15634f30c9865e880da3e1d1
># first bad commit: [721f4a6526daafca15634f30c9865e880da3e1d1] mm/memblock: remove empty dummy entry

-- 
Wei Yang
Help you, Help me

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

[Guenter Roeck]

On 6/20/24 18:07, Wei Yang wrote:
> On Thu, Jun 20, 2024 at 02:58:17PM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> 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>
>>>
>>
>> With this patch in linux-next, all microblaze qemu images fail to boot. Reverting it
>> fixes the problem.
>>
>> Bisect log is attached for reference.
>>
> 
> Thanks for the reporting.
> 
> Would you mind running the test to take a look?
> 
> It is in tools/testing/memblock. Simply make and ./main.
> 

This is a silent failure. There is no console output, so the image crashes
before it gets to that point.

Building microblaze:petalogix-s3adsp1800:initrd ... running ................R............. failed (silent)
------------
qemu log:
qemu-system-microblaze: terminating on signal 15 from pid 2343410 (/bin/bash)

Guenter

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

[Wei Yang]

On Thu, Jun 20, 2024 at 07:34:06PM -0700, Guenter Roeck wrote:
>On 6/20/24 18:07, Wei Yang wrote:
>> On Thu, Jun 20, 2024 at 02:58:17PM -0700, Guenter Roeck wrote:
>> > Hi,
>> > 
>> > 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>
>> > > 
>> > 
>> > With this patch in linux-next, all microblaze qemu images fail to boot. Reverting it
>> > fixes the problem.
>> > 
>> > Bisect log is attached for reference.
>> > 
>> 
>> Thanks for the reporting.
>> 
>> Would you mind running the test to take a look?
>> 
>> It is in tools/testing/memblock. Simply make and ./main.
>> 

This is a user-space test case, would you mind running it with this commit
applied?

>
>This is a silent failure. There is no console output, so the image crashes
>before it gets to that point.
>
>Building microblaze:petalogix-s3adsp1800:initrd ... running ................R............. failed (silent)
>------------
>qemu log:
>qemu-system-microblaze: terminating on signal 15 from pid 2343410 (/bin/bash)
>

Would you mind add kernel parameter memblock=debug without the commit applied?
If my understanding is correct, you can bootup without this commit, right?

>Guenter

-- 
Wei Yang
Help you, Help me

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

[Mike Rapoport]

(added microblaze maintainer)

On Fri, Jun 21, 2024 at 09:11:59PM -0700, Guenter Roeck wrote:
> On 6/21/24 16:06, Wei Yang wrote:
> > On Thu, Jun 20, 2024 at 07:34:06PM -0700, Guenter Roeck wrote:
> > > On 6/20/24 18:07, Wei Yang wrote:
> > > > On Thu, Jun 20, 2024 at 02:58:17PM -0700, Guenter Roeck wrote:
> > > > > Hi,
> > > > > 
> > > > > 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>
> > > > > > 
> > > > > 
> > > > > With this patch in linux-next, all microblaze qemu images fail to boot. Reverting it
> > > > > fixes the problem.
> > > 
> > > This is a silent failure. There is no console output, so the image crashes
> > > before it gets to that point.
> > > 
> > > Building microblaze:petalogix-s3adsp1800:initrd ... running ................R............. failed (silent)
> > > ------------
> > > qemu log:
> > > qemu-system-microblaze: terminating on signal 15 from pid 2343410 (/bin/bash)
> > 
> > Would you mind add kernel parameter memblock=debug without the commit applied?
> > If my understanding is correct, you can bootup without this commit, right?
> 
> Yes. With this change on top of linux-next:
> 
> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
> index 3827dc76edd8..1d3edac064ee 100644
> --- a/arch/microblaze/mm/init.c
> +++ b/arch/microblaze/mm/init.c
> @@ -195,7 +195,9 @@ asmlinkage void __init mmu_init(void)
> 
>         if (!memblock.reserved.cnt) {

I tried to understand what this was supposed to check, but this test was
there from day 1, so git archaeology didn't help :(

Anyway, it's perfectly fine to have any number of memblock reservations
here or no at all.

early_init_devtree() is called before mmu_init() and it sets up
memblock.memory via early_init_dt_scan() and even allows memblock
allocations. So the check for !memblock.reserved.cnt here seems wrong.

>                 pr_emerg("Error memory count\n");
> +#if 0
>                 machine_restart(NULL);
> +#endif
>         }
> 
> the log starts with:
> 
> random: crng init done
> Ramdisk addr 0x51923788,
> FDT at 0x51f43d88
> Error memory count
> MEMBLOCK configuration:
>  memory size = 0x10000000 reserved size = 0x015bb600
>  memory.cnt  = 0x1
>  memory[0x0]    [0x50000000-0x5fffffff], 0x10000000 bytes flags: 0x0
>  reserved.cnt  = 0x3
>  reserved[0x0]  [0x50000000-0x50f5cfff], 0x00f5d000 bytes flags: 0x0
>  reserved[0x1]  [0x510c0000-0x510fffff], 0x00040000 bytes flags: 0x0
>  reserved[0x2]  [0x51923788-0x51f41d87], 0x0061e600 bytes flags: 0x0
> Linux version 6.10.0-rc4-next-20240620-dirty (groeck@server.roeck-us.net) (microblaze-linux-gcc (GCC) 11.4.0, GNU ld (GNU Binutils) 2.40) #1 Fri Jun 21 21:00:40 PDT 2024
> setup_memory: max_mapnr: 0x10000
> setup_memory: min_low_pfn: 0x50000
> setup_memory: max_low_pfn: 0x60000
> setup_memory: max_pfn: 0x60000
> memblock_alloc_try_nid: 4096 bytes align=0x1000 nid=-1 from=0x00000000 max_addr=0x51100000 pte_alloc_one_kernel+0x50/0x64
> memblock_reserve: [0x510bf000-0x510bffff] memblock_alloc_range_nid+0x104/0x1d4
> 
> Guenter

I think simply removing the check for !memblock.reserved.cnt is the right
thing to do here:

From 975c5ba011330238444c82d0b171779c2156d2dc Mon Sep 17 00:00:00 2001
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Date: Sat, 22 Jun 2024 20:46:36 +0300
Subject: [PATCH] microblaze: don't treat zero reserved memory regions as error

Before commit 721f4a6526da ("mm/memblock: remove empty dummy entry") the
check for non-zero of memblock.reserved.cnt in mmu_init() would always
be true either because  memblock.reserved.cnt is initialized to 1 or
because there were memory reservations earlier.

The removal of dummy empty entry in memblock caused this check to fail
because now memblock.reserved.cnt is initialized to 0.

Remove the check for non-zero of memblock.reserved.cnt because it's
perfectly fine to have an empty memblock.reserved array that early in
boot.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
 arch/microblaze/mm/init.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
index 3827dc76edd8..4520c5741579 100644
--- a/arch/microblaze/mm/init.c
+++ b/arch/microblaze/mm/init.c
@@ -193,11 +193,6 @@ asmlinkage void __init mmu_init(void)
 {
 	unsigned int kstart, ksize;
 
-	if (!memblock.reserved.cnt) {
-		pr_emerg("Error memory count\n");
-		machine_restart(NULL);
-	}
-
 	if ((u32) memblock.memory.regions[0].size < 0x400000) {
 		pr_emerg("Memory must be greater than 4MB\n");
 		machine_restart(NULL);
-- 
2.43.0


-- 
Sincerely yours,
Mike.

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

[Wei Yang]

On Sat, Jun 22, 2024 at 09:16:23PM +0300, Mike Rapoport wrote:
>(added microblaze maintainer)
>
>On Fri, Jun 21, 2024 at 09:11:59PM -0700, Guenter Roeck wrote:
>> On 6/21/24 16:06, Wei Yang wrote:
>> > On Thu, Jun 20, 2024 at 07:34:06PM -0700, Guenter Roeck wrote:
>> > > On 6/20/24 18:07, Wei Yang wrote:
>> > > > On Thu, Jun 20, 2024 at 02:58:17PM -0700, Guenter Roeck wrote:
>> > > > > Hi,
>> > > > > 
>> > > > > 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>
>> > > > > > 
>> > > > > 
>> > > > > With this patch in linux-next, all microblaze qemu images fail to boot. Reverting it
>> > > > > fixes the problem.
>> > > 
>> > > This is a silent failure. There is no console output, so the image crashes
>> > > before it gets to that point.
>> > > 
>> > > Building microblaze:petalogix-s3adsp1800:initrd ... running ................R............. failed (silent)
>> > > ------------
>> > > qemu log:
>> > > qemu-system-microblaze: terminating on signal 15 from pid 2343410 (/bin/bash)
>> > 
>> > Would you mind add kernel parameter memblock=debug without the commit applied?
>> > If my understanding is correct, you can bootup without this commit, right?
>> 
>> Yes. With this change on top of linux-next:
>> 
>> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
>> index 3827dc76edd8..1d3edac064ee 100644
>> --- a/arch/microblaze/mm/init.c
>> +++ b/arch/microblaze/mm/init.c
>> @@ -195,7 +195,9 @@ asmlinkage void __init mmu_init(void)
>> 
>>         if (!memblock.reserved.cnt) {
>
>I tried to understand what this was supposed to check, but this test was
>there from day 1, so git archaeology didn't help :(
>
>Anyway, it's perfectly fine to have any number of memblock reservations
>here or no at all.
>
>early_init_devtree() is called before mmu_init() and it sets up
>memblock.memory via early_init_dt_scan() and even allows memblock
>allocations. So the check for !memblock.reserved.cnt here seems wrong.
>
>>                 pr_emerg("Error memory count\n");
>> +#if 0
>>                 machine_restart(NULL);
>> +#endif
>>         }
>> 
>> the log starts with:
>> 
>> random: crng init done
>> Ramdisk addr 0x51923788,
>> FDT at 0x51f43d88
>> Error memory count
>> MEMBLOCK configuration:
>>  memory size = 0x10000000 reserved size = 0x015bb600
>>  memory.cnt  = 0x1
>>  memory[0x0]    [0x50000000-0x5fffffff], 0x10000000 bytes flags: 0x0
>>  reserved.cnt  = 0x3
>>  reserved[0x0]  [0x50000000-0x50f5cfff], 0x00f5d000 bytes flags: 0x0
>>  reserved[0x1]  [0x510c0000-0x510fffff], 0x00040000 bytes flags: 0x0
>>  reserved[0x2]  [0x51923788-0x51f41d87], 0x0061e600 bytes flags: 0x0
>> Linux version 6.10.0-rc4-next-20240620-dirty (groeck@server.roeck-us.net) (microblaze-linux-gcc (GCC) 11.4.0, GNU ld (GNU Binutils) 2.40) #1 Fri Jun 21 21:00:40 PDT 2024
>> setup_memory: max_mapnr: 0x10000
>> setup_memory: min_low_pfn: 0x50000
>> setup_memory: max_low_pfn: 0x60000
>> setup_memory: max_pfn: 0x60000
>> memblock_alloc_try_nid: 4096 bytes align=0x1000 nid=-1 from=0x00000000 max_addr=0x51100000 pte_alloc_one_kernel+0x50/0x64
>> memblock_reserve: [0x510bf000-0x510bffff] memblock_alloc_range_nid+0x104/0x1d4
>> 
>> Guenter
>
>I think simply removing the check for !memblock.reserved.cnt is the right
>thing to do here:
>
>>From 975c5ba011330238444c82d0b171779c2156d2dc Mon Sep 17 00:00:00 2001
>From: "Mike Rapoport (IBM)" <rppt@kernel.org>
>Date: Sat, 22 Jun 2024 20:46:36 +0300
>Subject: [PATCH] microblaze: don't treat zero reserved memory regions as error
>
>Before commit 721f4a6526da ("mm/memblock: remove empty dummy entry") the
>check for non-zero of memblock.reserved.cnt in mmu_init() would always
>be true either because  memblock.reserved.cnt is initialized to 1 or
>because there were memory reservations earlier.
>
>The removal of dummy empty entry in memblock caused this check to fail
>because now memblock.reserved.cnt is initialized to 0.
>
>Remove the check for non-zero of memblock.reserved.cnt because it's
>perfectly fine to have an empty memblock.reserved array that early in
>boot.
>
>Reported-by: Guenter Roeck <linux@roeck-us.net>
>Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>

Thanks Mike :-)

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>---
> arch/microblaze/mm/init.c | 5 -----
> 1 file changed, 5 deletions(-)
>
>diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
>index 3827dc76edd8..4520c5741579 100644
>--- a/arch/microblaze/mm/init.c
>+++ b/arch/microblaze/mm/init.c
>@@ -193,11 +193,6 @@ asmlinkage void __init mmu_init(void)
> {
> 	unsigned int kstart, ksize;
> 
>-	if (!memblock.reserved.cnt) {
>-		pr_emerg("Error memory count\n");
>-		machine_restart(NULL);
>-	}
>-
> 	if ((u32) memblock.memory.regions[0].size < 0x400000) {
> 		pr_emerg("Memory must be greater than 4MB\n");
> 		machine_restart(NULL);
>-- 
>2.43.0
>
>
>-- 
>Sincerely yours,
>Mike.

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Thu, Jun 20, 2024 at 02:58:17PM -0700, Guenter Roeck wrote:
>Hi,
>
>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>
>> 
>
>With this patch in linux-next, all microblaze qemu images fail to boot. Reverting it
>fixes the problem.
>
>Bisect log is attached for reference.
>

Also, would you mind add kernel parameter memblock=debug and give me the log?

>Guenter
>
>---
># bad: [2102cb0d050d34d50b9642a3a50861787527e922] Add linux-next specific files for 20240619
># good: [6ba59ff4227927d3a8530fc2973b80e94b54d58f] Linux 6.10-rc4
>git bisect start 'HEAD' 'v6.10-rc4'
># good: [a8fa5261ec87d5aafd3211548d93008d5739457d] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
>git bisect good a8fa5261ec87d5aafd3211548d93008d5739457d
># good: [ee551f4db89753511a399b808db75654facec7c8] Merge branch 'for-linux-next' of https://gitlab.freedesktop.org/drm/i915/kernel
>git bisect good ee551f4db89753511a399b808db75654facec7c8
># good: [ec3557f4b791d72d93bfb69702d441d2c9f8cd0d] Merge branch 'next' of git://git.kernel.org/pub/scm/virt/kvm/kvm.git
>git bisect good ec3557f4b791d72d93bfb69702d441d2c9f8cd0d
># good: [48d51b3acbb237074014d498d76ea6b6ce5aed69] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git
>git bisect good 48d51b3acbb237074014d498d76ea6b6ce5aed69
># good: [59972b98583cd97febf9ecc576a706a7c5046278] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
>git bisect good 59972b98583cd97febf9ecc576a706a7c5046278
># good: [49aa15d761ce8976bb131f06886e89bd10cdb9fd] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching
>git bisect good 49aa15d761ce8976bb131f06886e89bd10cdb9fd
># bad: [129b9b07cd69885a804319c3fac82ef79e012e07] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/rppt/memblock.git
>git bisect bad 129b9b07cd69885a804319c3fac82ef79e012e07
># good: [344db92cbecc8da1f58d559926c61ceb72e72a03] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git
>git bisect good 344db92cbecc8da1f58d559926c61ceb72e72a03
># bad: [ce8ebb95439459f7e24b02c6943e278f46d2d328] mm/mm_init.c: get the highest zone directly
>git bisect bad ce8ebb95439459f7e24b02c6943e278f46d2d328
># bad: [1a879671bdfd14698a839f30de8e6d76e1e858fd] memblock tests: add memblock_overlaps_region_checks
>git bisect bad 1a879671bdfd14698a839f30de8e6d76e1e858fd
># bad: [3d3165193776ddacf59f101f0fa05cfab9f1a9ba] memblock tests: add memblock_reserve_all_locations_check()
>git bisect bad 3d3165193776ddacf59f101f0fa05cfab9f1a9ba
># bad: [721f4a6526daafca15634f30c9865e880da3e1d1] mm/memblock: remove empty dummy entry
>git bisect bad 721f4a6526daafca15634f30c9865e880da3e1d1
># first bad commit: [721f4a6526daafca15634f30c9865e880da3e1d1] mm/memblock: remove empty dummy entry

-- 
Wei Yang
Help you, Help me