Hi,
On Wed, Jan 04, 2023 at 07:43:36AM +0000, Aaron Thompson wrote:
If CONFIG_DEFERRED_STRUCT_PAGE_INIT is enabled, memblock_free_pages()
only releases pages to the buddy allocator if they are not in the
deferred range. This is correct for free pages (as defined by
for_each_free_mem_pfn_range_in_zone()) because free pages in the
deferred range will be initialized and released as part of the deferred
init process. memblock_free_pages() is called by memblock_free_late(),
which is used to free reserved ranges after memblock_free_all() has
run. memblock_free_all() initializes all pages in reserved ranges, and
To be precise, memblock_free_all() frees pages, or releases them to the
pages allocator, rather than initializes.
accordingly, those pages are not touched by the deferred init
process. This means that currently, if the pages that
memblock_free_late() intends to release are in the deferred range, they
will never be released to the buddy allocator. They will forever be
reserved.
In addition, memblock_free_pages() calls kmsan_memblock_free_pages(),
which is also correct for free pages but is not correct for reserved
pages. KMSAN metadata for reserved pages is initialized by
kmsan_init_shadow(), which runs shortly before memblock_free_all().
For both of these reasons, memblock_free_pages() should only be called
for free pages, and memblock_free_late() should call __free_pages_core()
directly instead.
Overall looks fine to me and I couldn't spot potential issues.
I'd appreciate if you add a paragraph about the actual issue with EFI boot
you described in the cover letter to the commit message.
Fixes: 3a80a7fa7989 ("mm: meminit: initialise a subset of struct pages if CONFIG_DEFERRED_STRUCT_PAGE_INIT is set")
Signed-off-by: Aaron Thompson <dev@xxxxxxxxxx>
---
mm/memblock.c | 2 +-
tools/testing/memblock/internal.h | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 511d4783dcf1..56a5b6086c50 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1640,7 +1640,7 @@ void __init memblock_free_late(phys_addr_t base, phys_addr_t size)
end = PFN_DOWN(base + size);
for (; cursor < end; cursor++) {
- memblock_free_pages(pfn_to_page(cursor), cursor, 0);
+ __free_pages_core(pfn_to_page(cursor), 0);
Please add a comment that explains why it is safe to call
__free_pages_core() here.
Something like
/*
* Reserved pages are always initialized by the end of
* memblock_free_all() either during memmap_init() or, with deferred
* initialization if struct page in reserve_bootmem_region()
*/
totalram_pages_inc();
}
}
diff --git a/tools/testing/memblock/internal.h b/tools/testing/memblock/internal.h
index fdb7f5db7308..85973e55489e 100644
--- a/tools/testing/memblock/internal.h
+++ b/tools/testing/memblock/internal.h
@@ -15,6 +15,10 @@ bool mirrored_kernelcore = false;
struct page {};
+void __free_pages_core(struct page *page, unsigned int order)
+{
+}
+
void memblock_free_pages(struct page *page, unsigned long pfn,
unsigned int order)
{
--
2.30.2