Re: [PATCH] kho: initialize tail pages for higher order folios properly

From: Pratyush Yadav
Date: Fri Jun 06 2025 - 12:31:40 EST

Hi Mike,

On Fri, Jun 06 2025, Mike Rapoport wrote:

> On Thu, Jun 05, 2025 at 07:11:41PM +0200, Pratyush Yadav wrote:
>> From: Pratyush Yadav <ptyadav@xxxxxxxxx>
>>
>> Currently, when restoring higher order folios, kho_restore_folio() only
>> calls prep_compound_page() on all the pages. That is not enough to
>> properly initialize the folios. The managed page count does not
>> get updated, the reserved flag does not get dropped, and page count does
>> not get initialized properly.
>>
>> Restoring a higher order folio with it results in the following BUG with
>> CONFIG_DEBUG_VM when attempting to free the folio:
>>
>> BUG: Bad page state in process test pfn:104e2b
>> page: refcount:1 mapcount:0 mapping:0000000000000000 index:0xffffffffffffffff pfn:0x104e2b
>> flags: 0x2fffff80000000(node=0|zone=2|lastcpupid=0x1fffff)
>> raw: 002fffff80000000 0000000000000000 00000000ffffffff 0000000000000000
>> raw: ffffffffffffffff 0000000000000000 00000001ffffffff 0000000000000000
>> page dumped because: nonzero _refcount
>> [...]
>> Call Trace:
>> <TASK>
>> dump_stack_lvl+0x4b/0x70
>> bad_page.cold+0x97/0xb2
>> __free_frozen_pages+0x616/0x850
>> [...]
>>
>> Combine the path for 0-order and higher order folios, initialize the
>> tail pages with a count of zero, and call adjust_managed_page_count() to
>> account for all the pages instead of just missing them.
>>
>> In addition, since all the KHO-preserved pages get marked with
>> MEMBLOCK_RSRV_NOINIT by deserialize_bitmap(), the reserved flag is not
>> actually set (as can also be seen from the flags of the dumped page in
>> the logs above). So drop the ClearPageReserved() calls.
>>
>> Fixes: fc33e4b44b271 ("kexec: enable KHO support for memory preservation")
>> Signed-off-by: Pratyush Yadav <ptyadav@xxxxxxxxx>
>> ---
>>
>> Side note: get_maintainers.pl for KHO only lists kexec@ as the mailing list.
>> Since KHO has a bunch of MM bits as well, should we also add linux-mm@ to its
>> MAINTAINERS entry?
>>
>> Adding linux-mm@ to this patch at least, in case MM people have an opinion on
>> this.
>>
>> kernel/kexec_handover.c | 29 +++++++++++++++++------------
>> 1 file changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/kexec_handover.c b/kernel/kexec_handover.c
>> index eb305e7e61296..5214ab27d1f8d 100644
>> --- a/kernel/kexec_handover.c
>> +++ b/kernel/kexec_handover.c
>> @@ -157,11 +157,21 @@ static int __kho_preserve_order(struct kho_mem_track *track, unsigned long pfn,
>> }
>>
>> /* almost as free_reserved_page(), just don't free the page */
>> -static void kho_restore_page(struct page *page)
>> +static void kho_restore_page(struct page *page, unsigned int order)
>> {
>> - ClearPageReserved(page);
>
> So now we don't clear PG_Reserved even on order-0 pages? ;-)

We don't need to. As I mentioned in the commit message as well,
PG_Reserved is never set for KHO pages since they are reserved with
MEMBLOCK_RSRV_NOINIT, so memmap_init_reserved_pages() skips over them.

To double-check, I added some quick prints to kho_restore_page():

/* Head page gets refcount of 1. */
set_page_count(page, 1);
printk("Head page flags: 0x%lx reserved: %d\n",
page->flags, PageReserved(page));

/* For higher order folios, tail pages get a page count of zero. */
for (i = 1; i < nr_pages; i++) {
printk("Tail page %u flags: 0x%lx reserved: %d\n",
i, (page+i)->flags, PageReserved(page+i));
set_page_count(page + i, 0);
}

And this is what I get:

[ 9.003187] Head page flags: 0x2fffff80000000 reserved: 0
[ 9.003730] Tail page 1 flags: 0x2fffff80000000 reserved: 0
[ 9.004229] Tail page 2 flags: 0x2fffff80000000 reserved: 0
[ 9.004740] Tail page 3 flags: 0x2fffff80000000 reserved: 0
[ 9.005265] Head page flags: 0x2fffff80000000 reserved: 0
[ 9.005759] Head page flags: 0x2fffff80000000 reserved: 0
[...]

So PG_Reserved is never set.

That said, while reading through some of the code, I noticed another
bug: because KHO reserves the preserved pages as NOINIT, with
CONFIG_DEFERRED_STRUCT_PAGE_INIT == n, all the pages get initialized
when memmap_init_range() is called from setup_arch (paging_init() on
x86). This happens before kho_memory_init(), so the KHO-preserved pages
are not marked as reserved to memblock yet.

With deferred page init, some pages might not get initialized early, and
get initialized after kho_memory_init(), by which time the KHO-preserved
pages are marked as reserved. So, deferred_init_maxorder() will skip
over those pages and leave them uninitialized.

And sure enough, doing the same with CONFIG_DEFERRED_STRUCT_PAGE_INIT ==
y results in:

[ 10.060842] Head page flags: 0x2fffff80000000 reserved: 0
[ 10.061387] Tail page 1 flags: 0x2fffff80000000 reserved: 0
[ 10.061902] Tail page 2 flags: 0x2fffff80000000 reserved: 0
[ 10.062400] Tail page 3 flags: 0x2fffff80000000 reserved: 0
[ 10.062924] page:00000000fb3dca54 is uninitialized and poisoned
[ 10.063494] page dumped because: VM_BUG_ON_PAGE(PagePoisoned(page))
[ 10.064190] ------------[ cut here ]------------
[ 10.064636] kernel BUG at ./include/linux/page-flags.h:571!
[ 10.065194] Oops: invalid opcode: 0000 [#1] SMP PTI
[ 10.065661] CPU: 2 UID: 0 PID: 1954 Comm: test Not tainted 6.15.0+ #297 PREEMPT(undef)
[ 10.066449] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
[ 10.067353] RIP: 0010:kho_restore_folio+0x4e/0x70
[...]

So we need to either also call init_deferred_page(), or remove the
memblock_reserved_mark_noinit() call in deserialize_bitmap(). And TBH, I
am not sure why KHO pages even need to be marked noinit in the first
place. Probably the only benefit would be if a large chunk of memory is
KHO-preserved, the pages can be initialized later on-demand, reducing
bootup time a bit.

What do you think? Should we drop noinit or call init_deferred_page()?
FWIW, my preference is to drop noinit, since init_deferred_page() is
__meminit and we would have to make sure it doesn't go away after boot.

>
>> - init_page_count(page);
>> - adjust_managed_page_count(page, 1);
>> + unsigned int i, nr_pages = (1 << order);
>
> Can you please declare 'i' inside the loop, looks nicer IMHO.

Ok, will do.

>
>> +
>> + /* Head page gets refcount of 1. */
>> + set_page_count(page, 1);
>
> ClearPageReserved(page) here?
>
>> +
>> + /* For higher order folios, tail pages get a page count of zero. */
>> + for (i = 1; i < nr_pages; i++)
>> + set_page_count(page + i, 0);
>
> and here?
>
>> +
>> + if (order > 0)
>> + prep_compound_page(page, order);
>> +
>> + adjust_managed_page_count(page, nr_pages);
>> }
>>
>> /**
[...]

--
Regards,
Pratyush Yadav