Received-SPF: pass (google.com: domain of linux-ext4+bounces-2098-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
To: Christian Brauner <brauner@kernel.org>, Nam Cao <namcao@linutronix.de>,
 Mike Rapoport <rppt@kernel.org>
Cc: Andreas Dilger <adilger@dilger.ca>, Al Viro <viro@zeniv.linux.org.uk>,
 linux-fsdevel <linux-fsdevel@vger.kernel.org>, Jan Kara <jack@suse.cz>,
 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
 linux-riscv@lists.infradead.org, Theodore Ts'o <tytso@mit.edu>, Ext4
 Developers List <linux-ext4@vger.kernel.org>, Conor Dooley
 <conor@kernel.org>, "Matthew Wilcox (Oracle)" <willy@infradead.org>,
 Anders Roxell <anders.roxell@linaro.org>, Alexandre Ghiti <alex@ghiti.fr>
Subject: Re: riscv32 EXT4 splat, 6.8 regression?
In-Reply-To: <20240416-deppen-gasleitung-8098fcfd6bbd@brauner>
References: <20240416-deppen-gasleitung-8098fcfd6bbd@brauner>
Date: Tue, 16 Apr 2024 13:02:20 +0200
Message-ID: <8734rlo9j7.fsf@all.your.base.are.belong.to.us>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Christian Brauner <brauner@kernel.org> writes:

> [Adding Mike who's knowledgeable in this area]

>> > Further, it seems like riscv32 indeed inserts a page like that to the
>> > buddy allocator, when the memblock is free'd:
>> >=20
>> >   | [<c024961c>] __free_one_page+0x2a4/0x3ea
>> >   | [<c024a448>] __free_pages_ok+0x158/0x3cc
>> >   | [<c024b1a4>] __free_pages_core+0xe8/0x12c
>> >   | [<c0c1435a>] memblock_free_pages+0x1a/0x22
>> >   | [<c0c17676>] memblock_free_all+0x1ee/0x278
>> >   | [<c0c050b0>] mem_init+0x10/0xa4
>> >   | [<c0c1447c>] mm_core_init+0x11a/0x2da
>> >   | [<c0c00bb6>] start_kernel+0x3c4/0x6de
>> >=20
>> > Here, a page with VA 0xfffff000 is a added to the freelist. We were ju=
st
>> > lucky (unlucky?) that page was used for the page cache.
>>=20
>> I just educated myself about memory mapping last night, so the below
>> may be complete nonsense. Take it with a grain of salt.
>>=20
>> In riscv's setup_bootmem(), we have this line:
>> 	max_low_pfn =3D max_pfn =3D PFN_DOWN(phys_ram_end);
>>=20
>> I think this is the root cause: max_low_pfn indicates the last page
>> to be mapped. Problem is: nothing prevents PFN_DOWN(phys_ram_end) from
>> getting mapped to the last page (0xfffff000). If max_low_pfn is mapped
>> to the last page, we get the reported problem.
>>=20
>> There seems to be some code to make sure the last page is not used
>> (the call to memblock_set_current_limit() right above this line). It is
>> unclear to me why this still lets the problem slip through.
>>=20
>> The fix is simple: never let max_low_pfn gets mapped to the last page.
>> The below patch fixes the problem for me. But I am not entirely sure if
>> this is the correct fix, further investigation needed.
>>=20
>> Best regards,
>> Nam
>>=20
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index fa34cf55037b..17cab0a52726 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -251,7 +251,8 @@ static void __init setup_bootmem(void)
>>  	}
>>=20=20
>>  	min_low_pfn =3D PFN_UP(phys_ram_base);
>> -	max_low_pfn =3D max_pfn =3D PFN_DOWN(phys_ram_end);
>> +	max_low_pfn =3D PFN_DOWN(memblock_get_current_limit());
>> +	max_pfn =3D PFN_DOWN(phys_ram_end);
>>  	high_memory =3D (void *)(__va(PFN_PHYS(max_low_pfn)));
>>=20=20
>>  	dma32_phys_limit =3D min(4UL * SZ_1G, (unsigned long)PFN_PHYS(max_low_=
pfn));

Yeah, AFAIU memblock_set_current_limit() only limits the allocation from
memblock. The "forbidden" page (PA 0xc03ff000 VA 0xfffff000) will still
be allowed in the zone.

I think your patch requires memblock_set_current_limit() is
unconditionally called, which currently is not done.

The hack I tried was (which seems to work):

--
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index fe8e159394d8..3a1f25d41794 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -245,8 +245,10 @@ static void __init setup_bootmem(void)
         */
        if (!IS_ENABLED(CONFIG_64BIT)) {
                max_mapped_addr =3D __pa(~(ulong)0);
-               if (max_mapped_addr =3D=3D (phys_ram_end - 1))
+               if (max_mapped_addr =3D=3D (phys_ram_end - 1)) {
                        memblock_set_current_limit(max_mapped_addr - 4096);
+                       phys_ram_end -=3D 4096;
+               }
        }
=20
        min_low_pfn =3D PFN_UP(phys_ram_base);
--

I'd really like to see an actual MM person (Mike or Alex?) have some
input here, and not simply my pasta-on-wall approach. ;-)


Bj=C3=B6rn