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

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

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