+ mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges.patch added to -mm tree
The patch titled Subject: mm: initialize struct pages in reserved regions outside of the zone ranges has been added to the -mm tree. Its filename is mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges.patch
This patch should soon appear at https://ozlabs.org/~akpm/mmots/broken-out/mm-initialize-struct-pages-in-rese... and later at https://ozlabs.org/~akpm/mmotm/broken-out/mm-initialize-struct-pages-in-rese...
Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next and is updated there every 3-4 working days
------------------------------------------------------ From: Andrea Arcangeli aarcange@redhat.com Subject: mm: initialize struct pages in reserved regions outside of the zone ranges
Without this change, the pfn 0 isn't in any zone spanned range, and it's also not in any memory.memblock range, so the struct page of pfn 0 wasn't initialized and the PagePoison remained set when reserve_bootmem_region called __SetPageReserved, inducing a silent boot failure with DEBUG_VM (and correctly so, because the crash signaled the nodeid/nid of pfn 0 would be again wrong).
There's no enforcement that all memblock.reserved ranges must overlap memblock.memory ranges, so the memblock.reserved ranges also require an explicit initialization and the zones ranges need to be extended to include all memblock.reserved ranges with struct pages too or they'll be left uninitialized with PagePoison as it happened to pfn 0.
Link: https://lkml.kernel.org/r/20201205013238.21663-2-aarcange@redhat.com Fixes: 73a6e474cb37 ("mm: memmap_init: iterate over memblock regions rather that check each PFN") Signed-off-by: Andrea Arcangeli aarcange@redhat.com Cc: Mike Rapoport rppt@linux.ibm.com Cc: Baoquan He bhe@redhat.com Cc: David Hildenbrand david@redhat.com Cc: Mel Gorman mgorman@suse.de Cc: Michal Hocko mhocko@kernel.org Cc: Qian Cai cai@lca.pw Cc: Vlastimil Babka vbabka@suse.cz Cc: stable@vger.kernel.org Signed-off-by: Andrew Morton akpm@linux-foundation.org ---
include/linux/memblock.h | 17 ++++++++--- mm/debug.c | 3 + mm/memblock.c | 4 +- mm/page_alloc.c | 57 +++++++++++++++++++++++++++++-------- 4 files changed, 63 insertions(+), 18 deletions(-)
--- a/include/linux/memblock.h~mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges +++ a/include/linux/memblock.h @@ -251,7 +251,8 @@ static inline bool memblock_is_nomap(str int memblock_search_pfn_nid(unsigned long pfn, unsigned long *start_pfn, unsigned long *end_pfn); void __next_mem_pfn_range(int *idx, int nid, unsigned long *out_start_pfn, - unsigned long *out_end_pfn, int *out_nid); + unsigned long *out_end_pfn, int *out_nid, + struct memblock_type *type);
/** * for_each_mem_pfn_range - early memory pfn range iterator @@ -263,9 +264,17 @@ void __next_mem_pfn_range(int *idx, int * * Walks over configured memory ranges. */ -#define for_each_mem_pfn_range(i, nid, p_start, p_end, p_nid) \ - for (i = -1, __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid); \ - i >= 0; __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid)) +#define for_each_mem_pfn_range(i, nid, p_start, p_end, p_nid) \ + for (i = -1, __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid, \ + &memblock.memory); \ + i >= 0; __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid, \ + &memblock.memory)) + +#define for_each_res_pfn_range(i, nid, p_start, p_end, p_nid) \ + for (i = -1, __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid, \ + &memblock.reserved); \ + i >= 0; __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid, \ + &memblock.reserved))
#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT void __next_mem_pfn_range_in_zone(u64 *idx, struct zone *zone, --- a/mm/debug.c~mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges +++ a/mm/debug.c @@ -64,7 +64,8 @@ void __dump_page(struct page *page, cons * dump_page() when detected. */ if (page_poisoned) { - pr_warn("page:%px is uninitialized and poisoned", page); + pr_warn("page:%px pfn:%ld is uninitialized and poisoned", + page, page_to_pfn(page)); goto hex_only; }
--- a/mm/memblock.c~mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges +++ a/mm/memblock.c @@ -1198,9 +1198,9 @@ void __init_memblock __next_mem_range_re */ void __init_memblock __next_mem_pfn_range(int *idx, int nid, unsigned long *out_start_pfn, - unsigned long *out_end_pfn, int *out_nid) + unsigned long *out_end_pfn, int *out_nid, + struct memblock_type *type) { - struct memblock_type *type = &memblock.memory; struct memblock_region *r; int r_nid;
--- a/mm/page_alloc.c~mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges +++ a/mm/page_alloc.c @@ -1458,6 +1458,7 @@ static void __meminit init_reserved_page { pg_data_t *pgdat; int nid, zid; + bool found = false;
if (!early_page_uninitialised(pfn)) return; @@ -1468,10 +1469,15 @@ static void __meminit init_reserved_page for (zid = 0; zid < MAX_NR_ZONES; zid++) { struct zone *zone = &pgdat->node_zones[zid];
- if (pfn >= zone->zone_start_pfn && pfn < zone_end_pfn(zone)) + if (pfn >= zone->zone_start_pfn && pfn < zone_end_pfn(zone)) { + found = true; break; + } } - __init_single_page(pfn_to_page(pfn), pfn, zid, nid); + if (likely(found)) + __init_single_page(pfn_to_page(pfn), pfn, zid, nid); + else + WARN_ON_ONCE(1); } #else static inline void init_reserved_page(unsigned long pfn) @@ -6227,7 +6233,7 @@ void __init __weak memmap_init(unsigned unsigned long zone, unsigned long range_start_pfn) { - unsigned long start_pfn, end_pfn, next_pfn = 0; + unsigned long start_pfn, end_pfn, prev_pfn = 0; unsigned long range_end_pfn = range_start_pfn + size; u64 pgcnt = 0; int i; @@ -6235,7 +6241,7 @@ void __init __weak memmap_init(unsigned for_each_mem_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) { start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn); end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn); - next_pfn = clamp(next_pfn, range_start_pfn, range_end_pfn); + prev_pfn = clamp(prev_pfn, range_start_pfn, range_end_pfn);
if (end_pfn > start_pfn) { size = end_pfn - start_pfn; @@ -6243,10 +6249,10 @@ void __init __weak memmap_init(unsigned MEMINIT_EARLY, NULL, MIGRATE_MOVABLE); }
- if (next_pfn < start_pfn) - pgcnt += init_unavailable_range(next_pfn, start_pfn, + if (prev_pfn < start_pfn) + pgcnt += init_unavailable_range(prev_pfn, start_pfn, zone, nid); - next_pfn = end_pfn; + prev_pfn = end_pfn; }
/* @@ -6256,12 +6262,31 @@ void __init __weak memmap_init(unsigned * considered initialized. Make sure that memmap has a well defined * state. */ - if (next_pfn < range_end_pfn) - pgcnt += init_unavailable_range(next_pfn, range_end_pfn, + if (prev_pfn < range_end_pfn) + pgcnt += init_unavailable_range(prev_pfn, range_end_pfn, zone, nid);
+ /* + * memblock.reserved isn't enforced to overlap with + * memblock.memory so initialize the struct pages for + * memblock.reserved too in case it wasn't overlapping. + * + * If any struct page associated with a memblock.reserved + * range isn't overlapping with a zone range, it'll be left + * uninitialized, ideally with PagePoison, and it'll be a more + * easily detectable error. + */ + for_each_res_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) { + start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn); + end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn); + + if (end_pfn > start_pfn) + pgcnt += init_unavailable_range(start_pfn, end_pfn, + zone, nid); + } + if (pgcnt) - pr_info("%s: Zeroed struct page in unavailable ranges: %lld\n", + pr_info("%s: pages in unavailable ranges: %lld\n", zone_names[zone], pgcnt); }
@@ -6499,6 +6524,10 @@ void __init get_pfn_range_for_nid(unsign *start_pfn = min(*start_pfn, this_start_pfn); *end_pfn = max(*end_pfn, this_end_pfn); } + for_each_res_pfn_range(i, nid, &this_start_pfn, &this_end_pfn, NULL) { + *start_pfn = min(*start_pfn, this_start_pfn); + *end_pfn = max(*end_pfn, this_end_pfn); + }
if (*start_pfn == -1UL) *start_pfn = 0; @@ -7126,7 +7155,13 @@ unsigned long __init node_map_pfn_alignm */ unsigned long __init find_min_pfn_with_active_regions(void) { - return PHYS_PFN(memblock_start_of_DRAM()); + /* + * reserved regions must be included so that their page + * structure can be part of a zone and obtain a valid zoneid + * before __SetPageReserved(). + */ + return min(PHYS_PFN(memblock_start_of_DRAM()), + PHYS_PFN(memblock.reserved.regions[0].base)); }
/* _
Patches currently in -mm which might be from aarcange@redhat.com are
mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges.patch
Hi,
On Sat, Dec 05, 2020 at 04:54:01PM -0800, akpm@linux-foundation.org wrote:
The patch titled Subject: mm: initialize struct pages in reserved regions outside of the zone ranges has been added to the -mm tree. Its filename is mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges.patch
This patch should soon appear at https://ozlabs.org/~akpm/mmots/broken-out/mm-initialize-struct-pages-in-rese... and later at https://ozlabs.org/~akpm/mmotm/broken-out/mm-initialize-struct-pages-in-rese...
Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next and is updated there every 3-4 working days
From: Andrea Arcangeli aarcange@redhat.com Subject: mm: initialize struct pages in reserved regions outside of the zone ranges
Without this change, the pfn 0 isn't in any zone spanned range, and it's also not in any memory.memblock range, so the struct page of pfn 0 wasn't initialized and the PagePoison remained set when reserve_bootmem_region called __SetPageReserved, inducing a silent boot failure with DEBUG_VM (and correctly so, because the crash signaled the nodeid/nid of pfn 0 would be again wrong).
There's no enforcement that all memblock.reserved ranges must overlap memblock.memory ranges, so the memblock.reserved ranges also require an explicit initialization and the zones ranges need to be extended to include all memblock.reserved ranges with struct pages too or they'll be left uninitialized with PagePoison as it happened to pfn 0.
Link: https://lkml.kernel.org/r/20201205013238.21663-2-aarcange@redhat.com Fixes: 73a6e474cb37 ("mm: memmap_init: iterate over memblock regions rather that check each PFN") Signed-off-by: Andrea Arcangeli aarcange@redhat.com Cc: Mike Rapoport rppt@linux.ibm.com Cc: Baoquan He bhe@redhat.com Cc: David Hildenbrand david@redhat.com Cc: Mel Gorman mgorman@suse.de Cc: Michal Hocko mhocko@kernel.org Cc: Qian Cai cai@lca.pw Cc: Vlastimil Babka vbabka@suse.cz Cc: stable@vger.kernel.org Signed-off-by: Andrew Morton akpm@linux-foundation.org
include/linux/memblock.h | 17 ++++++++--- mm/debug.c | 3 + mm/memblock.c | 4 +- mm/page_alloc.c | 57 +++++++++++++++++++++++++++++-------- 4 files changed, 63 insertions(+), 18 deletions(-)
I don't see why we need all this complexity when a simple fixup was enough.
--- a/include/linux/memblock.h~mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges +++ a/include/linux/memblock.h
...
--- a/mm/page_alloc.c~mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges +++ a/mm/page_alloc.c
...
@@ -6227,7 +6233,7 @@ void __init __weak memmap_init(unsigned unsigned long zone, unsigned long range_start_pfn) {
- unsigned long start_pfn, end_pfn, next_pfn = 0;
- unsigned long start_pfn, end_pfn, prev_pfn = 0; unsigned long range_end_pfn = range_start_pfn + size; u64 pgcnt = 0; int i;
@@ -6235,7 +6241,7 @@ void __init __weak memmap_init(unsigned for_each_mem_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) { start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn); end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn);
next_pfn = clamp(next_pfn, range_start_pfn, range_end_pfn);
prev_pfn = clamp(prev_pfn, range_start_pfn, range_end_pfn);if (end_pfn > start_pfn) { size = end_pfn - start_pfn; @@ -6243,10 +6249,10 @@ void __init __weak memmap_init(unsigned MEMINIT_EARLY, NULL, MIGRATE_MOVABLE); }
if (next_pfn < start_pfn)pgcnt += init_unavailable_range(next_pfn, start_pfn,
if (prev_pfn < start_pfn)pgcnt += init_unavailable_range(prev_pfn, start_pfn, zone, nid);
next_pfn = end_pfn;
prev_pfn = end_pfn;/* @@ -6256,12 +6262,31 @@ void __init __weak memmap_init(unsigned * considered initialized. Make sure that memmap has a well defined * state. */
- if (next_pfn < range_end_pfn)
pgcnt += init_unavailable_range(next_pfn, range_end_pfn,
- if (prev_pfn < range_end_pfn)
pgcnt += init_unavailable_range(prev_pfn, range_end_pfn, zone, nid);
- /*
* memblock.reserved isn't enforced to overlap with* memblock.memory so initialize the struct pages for* memblock.reserved too in case it wasn't overlapping.** If any struct page associated with a memblock.reserved* range isn't overlapping with a zone range, it'll be left* uninitialized, ideally with PagePoison, and it'll be a more* easily detectable error.*/- for_each_res_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn);end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn);if (end_pfn > start_pfn)pgcnt += init_unavailable_range(start_pfn, end_pfn,zone, nid);- }
This means we are going iterate over all memory allocated before free_area_ini() from memblock extra time. One time here and another time in reserve_bootmem_region(). And this can be substantial for CMA and alloc_large_system_hash().
- if (pgcnt)
pr_info("%s: Zeroed struct page in unavailable ranges: %lld\n",
pr_info("%s: pages in unavailable ranges: %lld\n", zone_names[zone], pgcnt);} @@ -6499,6 +6524,10 @@ void __init get_pfn_range_for_nid(unsign *start_pfn = min(*start_pfn, this_start_pfn); *end_pfn = max(*end_pfn, this_end_pfn); }
- for_each_res_pfn_range(i, nid, &this_start_pfn, &this_end_pfn, NULL) {
*start_pfn = min(*start_pfn, this_start_pfn);*end_pfn = max(*end_pfn, this_end_pfn);- }
if (*start_pfn == -1UL) *start_pfn = 0; @@ -7126,7 +7155,13 @@ unsigned long __init node_map_pfn_alignm */ unsigned long __init find_min_pfn_with_active_regions(void) {
- return PHYS_PFN(memblock_start_of_DRAM());
- /*
* reserved regions must be included so that their page* structure can be part of a zone and obtain a valid zoneid* before __SetPageReserved().*/- return min(PHYS_PFN(memblock_start_of_DRAM()),
PHYS_PFN(memblock.reserved.regions[0].base));
So this implies that reserved memory starts before memory. Don't you find this weird?
} /* _
Patches currently in -mm which might be from aarcange@redhat.com are
mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges.patch
On Sun, Dec 06, 2020 at 08:48:49AM +0200, Mike Rapoport wrote:
I don't see why we need all this complexity when a simple fixup was enough.
Note the memblock bug crashed my systems:
VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
Please note how zone_spans_pfn expands:
static inline bool zone_spans_pfn(const struct zone *zone, unsigned long pfn) { return zone->zone_start_pfn <= pfn && pfn < zone_end_pfn(zone); }
For pfn_to_page(0), the zone_start_pfn is 1 for DMA zone. pfn is 0.
How is the above not going to trigger a false positive once again with the simple fix?
Not just pfn 0 in fact, any pfn reserved that happens to be the at the start of each nid.
Doesn't the simple fix reintroduce the same broken invariant that was meant to be fixed by the patch that required the simple fix or it won't boot?
- /*
* memblock.reserved isn't enforced to overlap with* memblock.memory so initialize the struct pages for* memblock.reserved too in case it wasn't overlapping.** If any struct page associated with a memblock.reserved* range isn't overlapping with a zone range, it'll be left* uninitialized, ideally with PagePoison, and it'll be a more* easily detectable error.*/- for_each_res_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn);end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn);if (end_pfn > start_pfn)pgcnt += init_unavailable_range(start_pfn, end_pfn,zone, nid);- }
This means we are going iterate over all memory allocated before free_area_ini() from memblock extra time. One time here and another time in reserve_bootmem_region(). And this can be substantial for CMA and alloc_large_system_hash().
The above loops over memory.reserved only.
Now the simple fix also has to loops over some memblock.reserved or it still wouldn't boot.
You worry about memory.reserved which is bound by RAM.
I worry about holes that aren't bound by RAM.
The simple fix won't just restrict itself to the RAM in memblock.reserved, it'll also go goes over the holes (that's all it can do in order to cover memblock.reserved ranges without looking at it).
The simple fix doesn't only go over pfn 0, it can keep looping from last page in the previous nid to the first page in the next nid across regions that weren't even in memblock.reserved and that may be larger than any memory reserved region.
Reserved regions aren't holes, so the complex fix is more likely to work stable since unlike the simple fix, it won't waste any CPU over any non-RAM hole that isn't part of memblock.reserved nor memblock.memory.
In addition the "static unsigned long hole_start_pfn" of the simple fix also pushed complexity into the common code caller: it made memmap_init non thread safe and in addition it required the memmap_init to be called in paddr physical order or it won't even boot anymore.
Now maybe it's safe now, but these memmap already gets initialized in the background during boot, so the moment more than one thread does it in parallel the simple patch will break because it's non-reentrant.
@@ -7126,7 +7155,13 @@ unsigned long __init node_map_pfn_alignm */ unsigned long __init find_min_pfn_with_active_regions(void) {
- return PHYS_PFN(memblock_start_of_DRAM());
- /*
* reserved regions must be included so that their page* structure can be part of a zone and obtain a valid zoneid* before __SetPageReserved().*/- return min(PHYS_PFN(memblock_start_of_DRAM()),
PHYS_PFN(memblock.reserved.regions[0].base));So this implies that reserved memory starts before memory. Don't you find this weird?
That happens because the current API doesn't require overlap.
for (i = 0; i < e820_table->nr_entries; i++) { struct e820_entry *entry = &e820_table->entries[i];
end = entry->addr + entry->size; if (end != (resource_size_t)end) continue;
if (entry->type == E820_TYPE_SOFT_RESERVED) memblock_reserve(entry->addr, entry->size);
if (entry->type != E820_TYPE_RAM && entry->type != E820_TYPE_RESERVED_KERN) continue;
memblock_add(entry->addr, entry->size); }
The above is a more complex way to write the below, which will make it more clear there's never overlap:
if (entry->type == E820_TYPE_SOFT_RESERVED) memblock_reserve(entry->addr, entry->size); else if (entry->type == E820_TYPE_RAM || entry->type == E820_TYPE_RESERVED_KERN) memblock_add(entry->addr, entry->size);
If you want to enforce that there is always overlap and in turn a zone cannot start with a reserved pfn, then you could add a bugcheck if memblock.reserved doesn't overlap to an already registered memblock.memory range, and then the above e820 code above will break.
I thought the reason there couldn't be full overlap is the direct mapping issue so there's a very good reason for it.
So given there's current zero overlap, I'm not exactly sure what's weird that memblock.reserved starts before memblock.memory. The fact pfn 0 is reserved is a bios thing, weird or not weird it's not in our control.
I don't see any alternative on how to give a consistent zoneid to pfn 0, if zoneid 0 still starts at pfn 1 and you're not going to enforce full overlap between memblock.reserved and memblock.memory either.
One only alternative I can see is to leave PagePoison in there and add PagePoison or PageReserved checks all over the place starting from reserve_bootmem_region like this:
void __meminit reserve_bootmem_region(phys_addr_t start, phys_addr_t end) { unsigned long start_pfn = PFN_DOWN(start); unsigned long end_pfn = PFN_UP(end);
for (; start_pfn < end_pfn; start_pfn++) { if (pfn_valid(start_pfn)) { struct page *page = pfn_to_page(start_pfn);
+ if (PagePoison(page)) + continue
init_reserved_page(start_pfn);
The above is still preferable since then we could then add PagePoison checks all over the VM code like this:
VM_BUG_ON(!PagePoison(page) && page_to_pfn(page) >= page_zone(page)->zone_start_pfn)
The above PagePoison mention assumes we'd need to extend PagePoison to DEBUG_VM=n or switch to PageReserved or a NO_NODEID.
The whole point is we don't need to do any of the above since zones can include all reserved pages too so once the kernel finished booting, it'll be simpler if we have a valid zoneid so the invariant bugchecks will pass for all valid pfn and they'll stop crashing kernels with false positives.
Keeping all complexity in memblock common code and keeping the arch caller raw is not a bad thing since memblock is common code and it'll benefit all archs. I wouldn't go in the direction of 124049decbb121ec32742c94fb5d9d6bed8f24d8 once again.
Thanks, Andrea
On Sun, Dec 06, 2020 at 02:30:11PM -0500, Andrea Arcangeli wrote:
On Sun, Dec 06, 2020 at 08:48:49AM +0200, Mike Rapoport wrote:
I don't see why we need all this complexity when a simple fixup was enough.
Note the memblock bug crashed my systems:
VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
Please note how zone_spans_pfn expands:
static inline bool zone_spans_pfn(const struct zone *zone, unsigned long pfn) { return zone->zone_start_pfn <= pfn && pfn < zone_end_pfn(zone); }
For pfn_to_page(0), the zone_start_pfn is 1 for DMA zone. pfn is 0.
I don't know why you keep calling it "memblock bug" as pfn 0 was not part of DMA zone at least for ten years.
If you backport this VM_BUG_ON_PAGE() to 2.6.36 that has no memblock on x86 it will happily trigger there.
The problem, as I see it, in, hmm, questionable semantics for !E820_TYPE_RAM ranges on x86.
I keep asking this simple question again and again and I still have no clear "yes" or "no" answer:
Is PFN 0 on x86 memory?
Let's step back for a second and instead of blaming memblock in lack of enforcement for .memory and .reserved to overlap and talking about how memblock core was not robust enough we'll get to the point where we have clear defintions of what e820 ranges represent.
- /*
* memblock.reserved isn't enforced to overlap with* memblock.memory so initialize the struct pages for* memblock.reserved too in case it wasn't overlapping.** If any struct page associated with a memblock.reserved* range isn't overlapping with a zone range, it'll be left* uninitialized, ideally with PagePoison, and it'll be a more* easily detectable error.*/- for_each_res_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn);end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn);if (end_pfn > start_pfn)pgcnt += init_unavailable_range(start_pfn, end_pfn,zone, nid);- }
This means we are going iterate over all memory allocated before free_area_ini() from memblock extra time. One time here and another time in reserve_bootmem_region(). And this can be substantial for CMA and alloc_large_system_hash().
The above loops over memory.reserved only.
Right, this loops over memory.reserved which happens to include CMA and the memory allocated in alloc_large_system_hash(). This can be a lot. And memmap_init() runs before threads are available so this cannot be parallelized. The deferred memmap initialization starts much later than this.
@@ -7126,7 +7155,13 @@ unsigned long __init node_map_pfn_alignm */ unsigned long __init find_min_pfn_with_active_regions(void) {
- return PHYS_PFN(memblock_start_of_DRAM());
- /*
* reserved regions must be included so that their page* structure can be part of a zone and obtain a valid zoneid* before __SetPageReserved().*/- return min(PHYS_PFN(memblock_start_of_DRAM()),
PHYS_PFN(memblock.reserved.regions[0].base));So this implies that reserved memory starts before memory. Don't you find this weird?
The above is a more complex way to write the below, which will make it more clear there's never overlap:
if (entry->type == E820_TYPE_SOFT_RESERVED) memblock_reserve(entry->addr, entry->size); else if (entry->type == E820_TYPE_RAM || entry->type == E820_TYPE_RESERVED_KERN) memblock_add(entry->addr, entry->size);
So what about E820_TYPE_ACPI or E820_TYPE_NVS? How should they be treated?
It happend that we memblock_reserve() the first page, but what if the BIOS marks pfn 1 as E820_TYPE_ACPI? It's neither in .memory nor in .reserved. Does it belong to zone 0 and node 0?
Or, if, say, node 1 starts at 4G and the BIOS claimed several first pages of node 1 as type 20? What should we do then?
Thanks, Andrea
On Mon, Dec 07, 2020 at 06:50:37PM +0200, Mike Rapoport wrote:
On Sun, Dec 06, 2020 at 02:30:11PM -0500, Andrea Arcangeli wrote:
On Sun, Dec 06, 2020 at 08:48:49AM +0200, Mike Rapoport wrote:
I don't see why we need all this complexity when a simple fixup was enough.
Note the memblock bug crashed my systems:
VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
Please note how zone_spans_pfn expands:
static inline bool zone_spans_pfn(const struct zone *zone, unsigned long pfn) { return zone->zone_start_pfn <= pfn && pfn < zone_end_pfn(zone); }
For pfn_to_page(0), the zone_start_pfn is 1 for DMA zone. pfn is 0.
I don't know why you keep calling it "memblock bug" as pfn 0 was not part of DMA zone at least for ten years.
If you backport this VM_BUG_ON_PAGE() to 2.6.36 that has no memblock on x86 it will happily trigger there.
I guess it wasn't considered a bug until your patch that was meant to enforce this invariant for all pfn_valid was merged in -mm?
The problem, as I see it, in, hmm, questionable semantics for !E820_TYPE_RAM ranges on x86.
I keep asking this simple question again and again and I still have no clear "yes" or "no" answer:
Is PFN 0 on x86 memory?
The pfn is the same regardless if it's RAM or non RAM, so I don't think it makes any difference in this respect if it's RAM or not.
Let's step back for a second and instead of blaming memblock in lack of enforcement for .memory and .reserved to overlap and talking about how memblock core was not robust enough we'll get to the point where we have clear defintions of what e820 ranges represent.
I don't see why we should go back into the direction that the caller needs improvement like in commit 124049decbb121ec32742c94fb5d9d6bed8f24d8 and to try to massage the raw e820 data before giving it to the linux common code.
Even if you wanted to enforce overlap at all times (not possible because of the direct mapping issue) you could do it simply by calling memblock_add first thing in memblock_reserve (if it wasn't already fully overlapping) without the need to add an explicit memblock_add in the caller.
- /*
* memblock.reserved isn't enforced to overlap with* memblock.memory so initialize the struct pages for* memblock.reserved too in case it wasn't overlapping.** If any struct page associated with a memblock.reserved* range isn't overlapping with a zone range, it'll be left* uninitialized, ideally with PagePoison, and it'll be a more* easily detectable error.*/- for_each_res_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn);end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn);if (end_pfn > start_pfn)pgcnt += init_unavailable_range(start_pfn, end_pfn,zone, nid);- }
This means we are going iterate over all memory allocated before free_area_ini() from memblock extra time. One time here and another time in reserve_bootmem_region(). And this can be substantial for CMA and alloc_large_system_hash().
The above loops over memory.reserved only.
Right, this loops over memory.reserved which happens to include CMA and the memory allocated in alloc_large_system_hash(). This can be a lot. And memmap_init() runs before threads are available so this cannot be parallelized. The deferred memmap initialization starts much later than this.
Can you convert the memblock.memory and memblock.reserved to an interval tree? interval tree shall work perfectly since the switch to long mode, no need of dynamic RAM allocations for its inner working. There's no need to keep memblock a O(N) list.
Such change would benefit the code even if we remove the loop over memblock.reserved.
@@ -7126,7 +7155,13 @@ unsigned long __init node_map_pfn_alignm */ unsigned long __init find_min_pfn_with_active_regions(void) {
- return PHYS_PFN(memblock_start_of_DRAM());
- /*
* reserved regions must be included so that their page* structure can be part of a zone and obtain a valid zoneid* before __SetPageReserved().*/- return min(PHYS_PFN(memblock_start_of_DRAM()),
PHYS_PFN(memblock.reserved.regions[0].base));So this implies that reserved memory starts before memory. Don't you find this weird?
The above is a more complex way to write the below, which will make it more clear there's never overlap:
if (entry->type == E820_TYPE_SOFT_RESERVED) memblock_reserve(entry->addr, entry->size); else if (entry->type == E820_TYPE_RAM || entry->type == E820_TYPE_RESERVED_KERN) memblock_add(entry->addr, entry->size);So what about E820_TYPE_ACPI or E820_TYPE_NVS? How should they be treated?
It happend that we memblock_reserve() the first page, but what if the BIOS marks pfn 1 as E820_TYPE_ACPI? It's neither in .memory nor in .reserved. Does it belong to zone 0 and node 0?
Or, if, say, node 1 starts at 4G and the BIOS claimed several first pages of node 1 as type 20? What should we do then?
I see we still have dependencies on the caller to send down good data, which would be ideal to remove.
I guess before worrying of pfn 1 not being added to memblock.reserved, I'd worry about pfn 0 itself not being added to memblock.reserved.
My above worry about pfn 0 not being added to memblock.reserved doesn't change that:
- we still need to enlarge the DMA zone, even if pfn 0 is not added to memblock.reserved, if we want to really enforce the invariant that you can go to the start of the pageblock and pass the invariant on the first pfn, if any pfn in the first pageblock is in memblock.memory. So we also need to round down the start of the zone by the pageblock size. And adjust all range walks accordingly.
- Even after the rounddown, we still have to initialize the reserved ranges with pfn_valid or reserve_bootmem_region won't work at all if they don't happen to covered by the rounddown described above.
Still with the complex patch applied, if something goes wrong DEBUG_VM=y will make it reproducible, with the simple fix we return in non-reproducible land.
So I agree there's room for improvement and I believe the rounddown to the start of the pageblock is needed for those zones that have no adjacent region.
The important thing is that uninitialized pages are now left with PagePoison now, at least with DEBUG_VM=y. And when a zoneid is assigned to a page, that page is really is part of the zone. Both were false before, so things already improved in that direction :).
Thanks, Andrea
On Mon, Dec 07, 2020 at 09:57:37PM -0500, Andrea Arcangeli wrote:
On Mon, Dec 07, 2020 at 06:50:37PM +0200, Mike Rapoport wrote:
On Sun, Dec 06, 2020 at 02:30:11PM -0500, Andrea Arcangeli wrote:
On Sun, Dec 06, 2020 at 08:48:49AM +0200, Mike Rapoport wrote:
I don't see why we need all this complexity when a simple fixup was enough.
Note the memblock bug crashed my systems:
VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
Please note how zone_spans_pfn expands:
static inline bool zone_spans_pfn(const struct zone *zone, unsigned long pfn) { return zone->zone_start_pfn <= pfn && pfn < zone_end_pfn(zone); }
For pfn_to_page(0), the zone_start_pfn is 1 for DMA zone. pfn is 0.
I don't know why you keep calling it "memblock bug" as pfn 0 was not part of DMA zone at least for ten years.
If you backport this VM_BUG_ON_PAGE() to 2.6.36 that has no memblock on x86 it will happily trigger there.
I guess it wasn't considered a bug until your patch that was meant to enforce this invariant for all pfn_valid was merged in -mm?
What was not considered a bug? That node 0 and DMA zone on x86 started at pfn 1 and pfn 0 zone/node links were set by chance to the right values?
There was no intention to engfoce any invariant of overlaping or not ovelapping ranges in memblock.memory and memblock.reserved.
The change in 73a6e474cb376921a311786652782155eac2fdf0 was to change the loop over all pfns in a zone that among other things checked if that pfn is in memblock.memory to a loop over intersection of pfns in a zone with memblock.memory.
This exposed that before 73a6e474cb376921a311786652782155eac2fdf0: * small holes, like your "7a17b000-7a216fff : Unknown E820 type" get their zone/node links right because of an optimizaiton in __early_pfn_to_nid() that didn't check if a pfn is in memblock.memory for every pfn but rather cached the results. * on x86 pfn 0 was neither in node 0 nor in DMA zone because they both started with pfn 1; it got zone/node links set to 0 because that's what init_unavailable_range() did for all pfns in "unavailable ranges".
So, if you add something like this
static int check_pfn0(void) { unsigned long pfn = 0; struct page *page = pfn_to_page(pfn);
VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
return 0; } late_initcall(check_pfn0);
to an older kernel, this will blow up.
So, my guess is that we've never got to this check for pfn 0 and I really don't know how your system crashed with both my patches applied, unless you had a patch that checked pfn 0.
The problem, as I see it, in, hmm, questionable semantics for !E820_TYPE_RAM ranges on x86.
I keep asking this simple question again and again and I still have no clear "yes" or "no" answer:
Is PFN 0 on x86 memory?
The pfn is the same regardless if it's RAM or non RAM, so I don't think it makes any difference in this respect if it's RAM or not.
Let's step back for a second and instead of blaming memblock in lack of enforcement for .memory and .reserved to overlap and talking about how memblock core was not robust enough we'll get to the point where we have clear defintions of what e820 ranges represent.
I don't see why we should go back into the direction that the caller needs improvement like in commit 124049decbb121ec32742c94fb5d9d6bed8f24d8 and to try to massage the raw e820 data before giving it to the linux common code.
That's the entire point that the e820 data is massaged before it gets to the common code. 124049decbb121ec32742c94fb5d9d6bed8f24d8 tried to alter the way the raw e820 was transformed before going to the common code and my feeling it was a shot in the dark.
The current abstraction we have in the common code to describe physical memory is memblock and I want to make it explicit that every range in e820 table is a part of system memory and hence a part of memblock.memory.
Even if you wanted to enforce overlap at all times (not possible because of the direct mapping issue) you could do it simply by calling memblock_add first thing in memblock_reserve (if it wasn't already fully overlapping) without the need to add an explicit memblock_add in the caller.
The purpose here is not to enforce overlap at all times [and it is possible despite the direct mapping issues ;-) ]. The idea is to have the abstract description of the physical memory (i.e. memblock) as close as possible to actual layout. The "simple" code in e820__memblock_setup() only made things obfuscated and inconsistent.
If you have memory populated at address 0 you would expect that for_each_mem_pfn_range() will start from pfn 0.
In addition, and implicit call to memblock_add() in memblock_reserve() would be a waste of CPU time as most of the times memblock_reserve() is called as the part of memory allocation before page allocator is ready.
- /*
* memblock.reserved isn't enforced to overlap with* memblock.memory so initialize the struct pages for* memblock.reserved too in case it wasn't overlapping.** If any struct page associated with a memblock.reserved* range isn't overlapping with a zone range, it'll be left* uninitialized, ideally with PagePoison, and it'll be a more* easily detectable error.*/- for_each_res_pfn_range(i, nid, &start_pfn, &end_pfn, NULL) {
start_pfn = clamp(start_pfn, range_start_pfn, range_end_pfn);end_pfn = clamp(end_pfn, range_start_pfn, range_end_pfn);if (end_pfn > start_pfn)pgcnt += init_unavailable_range(start_pfn, end_pfn,zone, nid);- }
This means we are going iterate over all memory allocated before free_area_ini() from memblock extra time. One time here and another time in reserve_bootmem_region(). And this can be substantial for CMA and alloc_large_system_hash().
The above loops over memory.reserved only.
Right, this loops over memory.reserved which happens to include CMA and the memory allocated in alloc_large_system_hash(). This can be a lot. And memmap_init() runs before threads are available so this cannot be parallelized. The deferred memmap initialization starts much later than this.
Can you convert the memblock.memory and memblock.reserved to an interval tree? interval tree shall work perfectly since the switch to long mode, no need of dynamic RAM allocations for its inner working. There's no need to keep memblock a O(N) list.
Sorry, I was not clear. The penalty here is not the traversal of memblock.reserved array. The penalty is for redundant initialization of struct page for ranges memblock.reserved describes. For instance, the if user specified hugetlb_cma=nnG loop over memblock.reserved will cause redundant initialization pass over the pages in this nnG.
Such change would benefit the code even if we remove the loop over memblock.reserved.
@@ -7126,7 +7155,13 @@ unsigned long __init node_map_pfn_alignm */ unsigned long __init find_min_pfn_with_active_regions(void) {
- return PHYS_PFN(memblock_start_of_DRAM());
- /*
* reserved regions must be included so that their page* structure can be part of a zone and obtain a valid zoneid* before __SetPageReserved().*/- return min(PHYS_PFN(memblock_start_of_DRAM()),
PHYS_PFN(memblock.reserved.regions[0].base));So this implies that reserved memory starts before memory. Don't you find this weird?
The above is a more complex way to write the below, which will make it more clear there's never overlap:
if (entry->type == E820_TYPE_SOFT_RESERVED) memblock_reserve(entry->addr, entry->size); else if (entry->type == E820_TYPE_RAM || entry->type == E820_TYPE_RESERVED_KERN) memblock_add(entry->addr, entry->size);So what about E820_TYPE_ACPI or E820_TYPE_NVS? How should they be treated?
It happend that we memblock_reserve() the first page, but what if the BIOS marks pfn 1 as E820_TYPE_ACPI? It's neither in .memory nor in .reserved. Does it belong to zone 0 and node 0?
Or, if, say, node 1 starts at 4G and the BIOS claimed several first pages of node 1 as type 20? What should we do then?
I see we still have dependencies on the caller to send down good data, which would be ideal to remove.
Regardless of particular implementation, the generic code cannot detect physical memory layout, this is up to architecture to detect what memory banks are populated and provide this information to the generic code.
So we'll always have what you call "dependencies on the caller" here. The good thing is that we can fix "the caller" when we find it's wrong.
I guess before worrying of pfn 1 not being added to memblock.reserved, I'd worry about pfn 0 itself not being added to memblock.reserved.
My point was that with a bit different e820 table we may again hit a corner case that we didn't anticipate.
pfn 0 is in memblock.reserved by a coincidence and not by design and using memblock.reserved to detect zone/node boundaries is not reliable.
Moreover, if you look for usage of for_each_mem_pfn_range() in page_alloc.c, it's looks very much that it is presumed to iterate over *all* physical memory, including mysterious pfn 0.
Still with the complex patch applied, if something goes wrong DEBUG_VM=y will make it reproducible, with the simple fix we return in non-reproducible land.
I still think that the complex patch is going in the wrong direction. We cannot rely on memblock.reserved to calculate zones and nodes span.
This:
+ /* + * reserved regions must be included so that their page + * structure can be part of a zone and obtain a valid zoneid + * before __SetPageReserved(). + */ + return min(PHYS_PFN(memblock_start_of_DRAM()), + PHYS_PFN(memblock.reserved.regions[0].base));
is definitely a hack that worked because "the caller" has this:
/* * Make sure page 0 is always reserved because on systems with * L1TF its contents can be leaked to user processes. */ memblock_reserve(0, PAGE_SIZE);
So, what would have happen if L1TF was not yet disclosed? ;-)
Anyway, I think we need somthing like:
* add everything in e820 to memblock.memory while keeping any !E820_TYPE_RAM out of the direct map * refactor initialization of struct page for holes in memory layout to have proper state for struct pages not backed by actual memory * add validation of memory/nodes/zones to make sure the generic code can deal with data that architecture supplied; this maybe only for DEBUG_VM=y case.
Thanks, Andrea
On Tue, Dec 08, 2020 at 11:46:14PM +0200, Mike Rapoport wrote:
On Mon, Dec 07, 2020 at 09:57:37PM -0500, Andrea Arcangeli wrote:
On Mon, Dec 07, 2020 at 06:50:37PM +0200, Mike Rapoport wrote:
On Sun, Dec 06, 2020 at 02:30:11PM -0500, Andrea Arcangeli wrote:
On Sun, Dec 06, 2020 at 08:48:49AM +0200, Mike Rapoport wrote:
I don't see why we need all this complexity when a simple fixup was enough.
Note the memblock bug crashed my systems:
VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
Please note how zone_spans_pfn expands:
static inline bool zone_spans_pfn(const struct zone *zone, unsigned long pfn) { return zone->zone_start_pfn <= pfn && pfn < zone_end_pfn(zone); }
For pfn_to_page(0), the zone_start_pfn is 1 for DMA zone. pfn is 0.
I don't know why you keep calling it "memblock bug" as pfn 0 was not part of DMA zone at least for ten years.
If you backport this VM_BUG_ON_PAGE() to 2.6.36 that has no memblock on x86 it will happily trigger there.
I guess it wasn't considered a bug until your patch that was meant to enforce this invariant for all pfn_valid was merged in -mm?
What was not considered a bug? That node 0 and DMA zone on x86 started at pfn 1 and pfn 0 zone/node links were set by chance to the right values?
Well, I don't think zoneid 0 is the "right value" for the pfn 0 since zoneid 0 DMA start from pfn 1.
Now if you rephrase the above with "were set to the wrong values", then the answer is yes.
There was no intention to engfoce any invariant of overlaping or not ovelapping ranges in memblock.memory and memblock.reserved.
The change in 73a6e474cb376921a311786652782155eac2fdf0 was to change the loop over all pfns in a zone that among other things checked if that pfn is in memblock.memory to a loop over intersection of pfns in a zone with memblock.memory.
This exposed that before 73a6e474cb376921a311786652782155eac2fdf0:
- small holes, like your "7a17b000-7a216fff : Unknown E820 type" get
their zone/node links right because of an optimizaiton in __early_pfn_to_nid() that didn't check if a pfn is in memblock.memory for every pfn but rather cached the results.
- on x86 pfn 0 was neither in node 0 nor in DMA zone because they both
started with pfn 1; it got zone/node links set to 0 because that's what init_unavailable_range() did for all pfns in "unavailable ranges".
So, if you add something like this
static int check_pfn0(void) { unsigned long pfn = 0; struct page *page = pfn_to_page(pfn);
VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn), page);
return 0; } late_initcall(check_pfn0);
to an older kernel, this will blow up.
Some older kernel wouldn't fail just at pfn 0. In v5.8/9 the above bugcheck crashes on pfn > 0.
In all versions before your https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org , worrying at pfn 0 and the first reserved pfn in each nid would be just window dressing.
Now the moment you apply https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org then pfn 0 and all other pfn at the start of each nid that still break the above bugcheck invariant, gets the focus because they're thing still breaking the above bugcheck, the very bugcheck crashing v5.8/9.
So, my guess is that we've never got to this check for pfn 0 and I really don't know how your system crashed with both my patches applied, unless you had a patch that checked pfn 0.
I thought I already mentioned it's the __SetPageReserved that checks PagePoison and the __SetPageReserved is issued on all struct page in memblock.reserved ranges.
You're calling __SetPageReserved on each one of the reserved pages. If you didn't go over each one of the struct page to initialize them and __SetPageReserved them, then https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org would be capable of booting without the simple fix too.
https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org is correctly failing boot, because what was missing was a proper zoneid initialization that would wipe the PagePoison away, on all memblock.reserved ranges that aren't overlapping on memblock.memory ranges, which is precisely what the complex patch does.
The problem, as I see it, in, hmm, questionable semantics for !E820_TYPE_RAM ranges on x86.
I keep asking this simple question again and again and I still have no clear "yes" or "no" answer:
Is PFN 0 on x86 memory?
The pfn is the same regardless if it's RAM or non RAM, so I don't think it makes any difference in this respect if it's RAM or not.
Let's step back for a second and instead of blaming memblock in lack of enforcement for .memory and .reserved to overlap and talking about how memblock core was not robust enough we'll get to the point where we have clear defintions of what e820 ranges represent.
I don't see why we should go back into the direction that the caller needs improvement like in commit 124049decbb121ec32742c94fb5d9d6bed8f24d8 and to try to massage the raw e820 data before giving it to the linux common code.
That's the entire point that the e820 data is massaged before it gets to the common code. 124049decbb121ec32742c94fb5d9d6bed8f24d8 tried to alter the way the raw e820 was transformed before going to the common code and my feeling it was a shot in the dark.
The current abstraction we have in the common code to describe physical memory is memblock and I want to make it explicit that every range in e820 table is a part of system memory and hence a part of memblock.memory.
What's the advantage of not initializing the struct pages in the memblock.reserved ranges, if you add all memblock.reserved to memblock.memory and you still initialize all memblock.memory then?
Isn't the cost of the loop and number of pfn being initialized then identical to the complex patch then?
What you suggest would require adding extra information to flag which ranges must not have a direct mapping, but that information is already in memblock today, for each range in memblock_reserved but not in memblock.memory or did I misunderstand how that no-direct-map detail works?
I see nothing wrong in adding memblock.reserved to memblock.memory, but the total information in kernel memory will remain the same, it's just a different representation that will require more CPU and RAM to handle, not that it matters. All it matters is what is simpler in the code and the current version looks simpler. Or do you expect the version that enforces overlap to reduce the kernel code?
The purpose here is not to enforce overlap at all times [and it is possible despite the direct mapping issues ;-) ]. The idea is to have the abstract description of the physical memory (i.e. memblock) as close as possible to actual layout. The "simple" code in e820__memblock_setup() only made things obfuscated and inconsistent.
If you have memory populated at address 0 you would expect that for_each_mem_pfn_range() will start from pfn 0.
Specifically which code do you envision that needs to get RAM pfn in physical order and cannot call:
for_each_mem_pfn_range(pfn)) handle(pfn); for_each_res_pfn_range(pfn)) handle(pfn);
If you need it in physical order immediately then having memblock.memory including memblock.reserve would simplify things for sure, but who needs that when it runs even exactly at the same speed?
In addition, and implicit call to memblock_add() in memblock_reserve() would be a waste of CPU time as most of the times memblock_reserve() is called as the part of memory allocation before page allocator is ready.
Well, given such CPU time is so little, I would still spend that CPU time to do an enforcement and bugcheck and manual call of memblock_add if any memblock_reserve has no full overlap.
So I'd rather spend the CPU time to do the work itself and not having to risk kernel-crashing issue if any of the callers got a future patch done wrong or if the bios maps aren't done fully right after a bios upgrade.
How much CPU time we're talking about here? Those aren't the computations on the pfn but on the ranges and there aren't going to be million of e820 entries.
Even if you envision million of e820 entries, the first thing to do then to save CPU time in memblock is drop all lists and replace them with interval trees based on rbtrees, so the complexity of the later loops that intersect the memblock ranges with the nid-zone ranges, will go in O(log(N)) where N is in the number of e820 ranges. Leaving memblock at risk from a buggy e820 caller is not going to save nearly as much CPU as such data structure computation complexity enhancement in my view. memblock_add then will also run in O(log(N)).
Sorry, I was not clear. The penalty here is not the traversal of memblock.reserved array. The penalty is for redundant initialization of struct page for ranges memblock.reserved describes.
But that's the fix... How can you avoid removing the PagePoison for all pfn_valid in memblock.reserved, when the crash of https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org is in __SetPageReserved of reserve_bootmem_region?
For instance, the if user specified hugetlb_cma=nnG loop over memblock.reserved will cause redundant initialization pass over the pages in this nnG.
And if that didn't happen, then I'm left wondering if free_low_memory_core_early would crash again with specified hugetlb_cma=nnG, not just in pfn 0 anymore even with the simple fix applied.
Where does PagePoison get removed from the CMA range, or is somehow __SetPageReserved not called with the simple fix applied on the CMA reserved range?
The important thing that the simple fix breaks (and get us back to square one) is that after https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org we're finally guaranteed each pfn_valid gets either a "right zoneid" or PagePosion. It is great in that respect and I'd rather not break such guarantee again with the simple fix that may still not boot with CMA region reservation and will also return breaking the check_pfn0 bugcheck above.
Could you test your simple fix with DEBUG_VM=y and hugetlb_cma=nnG, does it boot? While I'm not entirely sure how the simple fix can boot with DEBUG_VM=y and hugetlb_cma=nnG (I don't rule out it might boot), I'm pretty confident my complex fix will still boot fine with hugetlb_cma=nnG.
Regardless of particular implementation, the generic code cannot detect physical memory layout, this is up to architecture to detect what memory banks are populated and provide this information to the generic code.
So we'll always have what you call "dependencies on the caller" here. The good thing is that we can fix "the caller" when we find it's wrong.
The caller that massages the e820 bios table, is still software. Software can go from arch code to common code. There's no asm or arch dependency in there.
The caller in my view needs to know what e820 is and send down the raw info... it's a 1:1 API translation, the caller massaging of the data can all go in the callee and benefit all callers.
Alternatively, if the caller logic doesn't go from arch code to common code for whatever reason, the common code can still put the caller logic in the common code and use it to verify the caller did everything right, fixup if it didn't and warn in the logs about it, using the same software algo the caller uses to massage the e820 data.
The benefit is once the callee does all validation, then all archs that do a mistake will be told and boot will not fail and it'll be more likely to boot stable even in presence of inconsistent hardware tables.
The whole point of the above is to reduce the chance that we either have to keep cut-and-pasting N times the same logic in all callers, or that in the future a subtle change in the arch code or a subtle difference in the bios tables (kind of precisely what's crashing v5.8 and v5.9) will cause the kernel to boot unstable in a non easily reproducible way.
Looking the current e820 code it looks it's already expecting all validation to happen in the callee.
I guess before worrying of pfn 1 not being added to memblock.reserved, I'd worry about pfn 0 itself not being added to memblock.reserved.
My point was that with a bit different e820 table we may again hit a corner case that we didn't anticipate.
pfn 0 is in memblock.reserved by a coincidence and not by design and using memblock.reserved to detect zone/node boundaries is not reliable.
Moreover, if you look for usage of for_each_mem_pfn_range() in page_alloc.c, it's looks very much that it is presumed to iterate over *all* physical memory, including mysterious pfn 0.
All the complex patch does it that it guarantees all the reserved pages can get a "right" zoneid.
Then we certainly should consider rounding it down by the pageblock in case pfn 0 isn't reserved.
In fact if you add all memblock.reserved ranges to memblock.memory, you'll obtain exactly the same result in the zone_start_pfn as with the complex patch in the zone_start_pfn calculation and the exact same issue of PagePoison being left on pfn 0 will happen if pfn 0 isn't added to memblock.memory with { memblock_add; memblock_reserved }.
Still with the complex patch applied, if something goes wrong DEBUG_VM=y will make it reproducible, with the simple fix we return in non-reproducible land.
I still think that the complex patch is going in the wrong direction. We cannot rely on memblock.reserved to calculate zones and nodes span.
This:
/** reserved regions must be included so that their page* structure can be part of a zone and obtain a valid zoneid* before __SetPageReserved().*/return min(PHYS_PFN(memblock_start_of_DRAM()),PHYS_PFN(memblock.reserved.regions[0].base));is definitely a hack that worked because "the caller" has this:
/* * Make sure page 0 is always reserved because on systems with * L1TF its contents can be leaked to user processes. */ memblock_reserve(0, PAGE_SIZE);
So, what would have happen if L1TF was not yet disclosed? ;-)
It's actually fine thing to delete the above memblock_reserve(0, PAGE_SIZE) for all testing so we can move into the remaining issues.
After deleting the memblock_reserve of pfn 0, the VM_BUG_ON_PAGE will show PagePoison in page->flags in the bugcheck above, which is preferable than having a wrong zone id. In addition PagePoison will be more detectable error than a wrong zone id so it's more likely to notify us something fundamentally went wrong in memblock.
With simple fix we'll be left wondering once again as it happened before https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org .
Second, the removal of memblock_reserve(0, PAGE_SIZE) is the right test to do to verify and exercise the needed round_down to the start of the pageblock (if there's no adjacent zone preceding it) orthogonal enhancement that would be needed anyway even if we only looked at memblock.memory there (again to avoid leaving PagePoison or setting ti to a zoneid that won't pass the check_pfn0).
The end result of the "hack" is precisely what you obtain if you add all memblock.reserved to memblock.memory, except it doesn't require to add memblock.reserved to memblock.memory.
Thanks, Andrea
On Tue, Dec 08, 2020 at 06:13:47PM -0500, Andrea Arcangeli wrote:
On Tue, Dec 08, 2020 at 11:46:14PM +0200, Mike Rapoport wrote:
Sorry, I was not clear. The penalty here is not the traversal of memblock.reserved array. The penalty is for redundant initialization of struct page for ranges memblock.reserved describes.
But that's the fix... How can you avoid removing the PagePoison for all pfn_valid in memblock.reserved, when the crash of https://lore.kernel.org/lkml/20201201181502.2340-1-rppt@kernel.org is in __SetPageReserved of reserve_bootmem_region?
With your fix the pages in the memblock.reserved that intersects with memblock.memory will be initialized twice: first time in for_each_mem_pfn_range() and the second time in for_each_res_pfn_range()
For instance, the if user specified hugetlb_cma=nnG loop over memblock.reserved will cause redundant initialization pass over the pages in this nnG.
And if that didn't happen, then I'm left wondering if free_low_memory_core_early would crash again with specified hugetlb_cma=nnG, not just in pfn 0 anymore even with the simple fix applied.
Where does PagePoison get removed from the CMA range, or is somehow __SetPageReserved not called with the simple fix applied on the CMA reserved range?
CMA range would be in both memblock.memory and memblock.reserved. So the pages for it will be initialized in for_each_mem_pfn_range().
Regardless of particular implementation, the generic code cannot detect physical memory layout, this is up to architecture to detect what memory banks are populated and provide this information to the generic code.
So we'll always have what you call "dependencies on the caller" here. The good thing is that we can fix "the caller" when we find it's wrong.
The caller that massages the e820 bios table, is still software. Software can go from arch code to common code. There's no asm or arch dependency in there.
The caller in my view needs to know what e820 is and send down the raw info... it's a 1:1 API translation, the caller massaging of the data can all go in the callee and benefit all callers.
Well, the translation between e820 and memblock could been 1:1, but it is not. I think e820 in more broadly x86 memory initialization could benifit from more straightforward translation to memblock. For instance, I don't see why the pfn 0 should be marked as reserved in both e820 and memblock and why some of the e820 reserved areas never make it to memblock.reserved.
The benefit is once the callee does all validation, then all archs that do a mistake will be told and boot will not fail and it'll be more likely to boot stable even in presence of inconsistent hardware tables.
That's true. Having the generic code more robust will benifit everybody.
I guess before worrying of pfn 1 not being added to memblock.reserved, I'd worry about pfn 0 itself not being added to memblock.reserved.
My point was that with a bit different e820 table we may again hit a corner case that we didn't anticipate.
pfn 0 is in memblock.reserved by a coincidence and not by design and using memblock.reserved to detect zone/node boundaries is not reliable.
Moreover, if you look for usage of for_each_mem_pfn_range() in page_alloc.c, it's looks very much that it is presumed to iterate over *all* physical memory, including mysterious pfn 0.
All the complex patch does it that it guarantees all the reserved pages can get a "right" zoneid.
Then we certainly should consider rounding it down by the pageblock in case pfn 0 isn't reserved.
In fact if you add all memblock.reserved ranges to memblock.memory, you'll obtain exactly the same result in the zone_start_pfn as with the complex patch in the zone_start_pfn calculation and the exact same issue of PagePoison being left on pfn 0 will happen if pfn 0 isn't added to memblock.memory with { memblock_add; memblock_reserved }.
Still with the complex patch applied, if something goes wrong DEBUG_VM=y will make it reproducible, with the simple fix we return in non-reproducible land.
I still think that the complex patch is going in the wrong direction. We cannot rely on memblock.reserved to calculate zones and nodes span.
This:
/** reserved regions must be included so that their page* structure can be part of a zone and obtain a valid zoneid* before __SetPageReserved().*/return min(PHYS_PFN(memblock_start_of_DRAM()),PHYS_PFN(memblock.reserved.regions[0].base));is definitely a hack that worked because "the caller" has this:
/* * Make sure page 0 is always reserved because on systems with * L1TF its contents can be leaked to user processes. */ memblock_reserve(0, PAGE_SIZE);
So, what would have happen if L1TF was not yet disclosed? ;-)
It's actually fine thing to delete the above memblock_reserve(0, PAGE_SIZE) for all testing so we can move into the remaining issues.
There are few more places in arch/x86/kernel/setup.c that memblock_reserve() one or several pages from address 0 :-)
The end result of the "hack" is precisely what you obtain if you add all memblock.reserved to memblock.memory, except it doesn't require to add memblock.reserved to memblock.memory.
I still do not agree that there can be minimum between memblock_start_of_DRAM() and anything else. I think it's semantically wrong.
And I really dislike addition of memblock.reserved traversals, mostly because of the areas that actually overlap.
However, I do agree that adding non-overlaping part of memblock.reserved to memblock.memory will make the generic code more robust. I just don't want to do this implicitly by calling memblock_add() from memblock_reserve(). I think the cleaner way is to join them just before free_area_init() starts zone/node size detection.
Thanks, Andrea
On 06.12.20 01:54, akpm@linux-foundation.org wrote:
The patch titled Subject: mm: initialize struct pages in reserved regions outside of the zone ranges has been added to the -mm tree. Its filename is mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges.patch
This patch should soon appear at https://ozlabs.org/~akpm/mmots/broken-out/mm-initialize-struct-pages-in-rese... and later at https://ozlabs.org/~akpm/mmotm/broken-out/mm-initialize-struct-pages-in-rese...
Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next and is updated there every 3-4 working days
From: Andrea Arcangeli aarcange@redhat.com Subject: mm: initialize struct pages in reserved regions outside of the zone ranges
Without this change, the pfn 0 isn't in any zone spanned range, and it's also not in any memory.memblock range, so the struct page of pfn 0 wasn't initialized and the PagePoison remained set when reserve_bootmem_region called __SetPageReserved, inducing a silent boot failure with DEBUG_VM (and correctly so, because the crash signaled the nodeid/nid of pfn 0 would be again wrong).
There's no enforcement that all memblock.reserved ranges must overlap memblock.memory ranges, so the memblock.reserved ranges also require an explicit initialization and the zones ranges need to be extended to include all memblock.reserved ranges with struct pages too or they'll be left uninitialized with PagePoison as it happened to pfn 0.
Link: https://lkml.kernel.org/r/20201205013238.21663-2-aarcange@redhat.com Fixes: 73a6e474cb37 ("mm: memmap_init: iterate over memblock regions rather that check each PFN") Signed-off-by: Andrea Arcangeli aarcange@redhat.com Cc: Mike Rapoport rppt@linux.ibm.com Cc: Baoquan He bhe@redhat.com Cc: David Hildenbrand david@redhat.com Cc: Mel Gorman mgorman@suse.de Cc: Michal Hocko mhocko@kernel.org Cc: Qian Cai cai@lca.pw Cc: Vlastimil Babka vbabka@suse.cz Cc: stable@vger.kernel.org Signed-off-by: Andrew Morton akpm@linux-foundation.org
[...]
I've lost track which patches are we considering right now to solve the overall issue. I'd appreciate a proper patch series with all relevant patches.
On Mon, Dec 07, 2020 at 09:58:37AM +0100, David Hildenbrand wrote:
On 06.12.20 01:54, akpm@linux-foundation.org wrote:
The patch titled Subject: mm: initialize struct pages in reserved regions outside of the zone ranges has been added to the -mm tree. Its filename is mm-initialize-struct-pages-in-reserved-regions-outside-of-the-zone-ranges.patch
This patch should soon appear at https://ozlabs.org/~akpm/mmots/broken-out/mm-initialize-struct-pages-in-rese... and later at https://ozlabs.org/~akpm/mmotm/broken-out/mm-initialize-struct-pages-in-rese...
Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next and is updated there every 3-4 working days
From: Andrea Arcangeli aarcange@redhat.com Subject: mm: initialize struct pages in reserved regions outside of the zone ranges
Without this change, the pfn 0 isn't in any zone spanned range, and it's also not in any memory.memblock range, so the struct page of pfn 0 wasn't initialized and the PagePoison remained set when reserve_bootmem_region called __SetPageReserved, inducing a silent boot failure with DEBUG_VM (and correctly so, because the crash signaled the nodeid/nid of pfn 0 would be again wrong).
There's no enforcement that all memblock.reserved ranges must overlap memblock.memory ranges, so the memblock.reserved ranges also require an explicit initialization and the zones ranges need to be extended to include all memblock.reserved ranges with struct pages too or they'll be left uninitialized with PagePoison as it happened to pfn 0.
Link: https://lkml.kernel.org/r/20201205013238.21663-2-aarcange@redhat.com Fixes: 73a6e474cb37 ("mm: memmap_init: iterate over memblock regions rather that check each PFN") Signed-off-by: Andrea Arcangeli aarcange@redhat.com Cc: Mike Rapoport rppt@linux.ibm.com Cc: Baoquan He bhe@redhat.com Cc: David Hildenbrand david@redhat.com Cc: Mel Gorman mgorman@suse.de Cc: Michal Hocko mhocko@kernel.org Cc: Qian Cai cai@lca.pw Cc: Vlastimil Babka vbabka@suse.cz Cc: stable@vger.kernel.org Signed-off-by: Andrew Morton akpm@linux-foundation.org
[...]
I've lost track which patches are we considering right now to solve the overall issue. I'd appreciate a proper patch series with all relevant patches.
I think none of the patches is 100% right. I'll try to send a new version today.
-- Thanks,
David / dhildenb
linux-stable-mirror@lists.linaro.org
-
akpm＠linux-foundation.org -
Andrea Arcangeli -
David Hildenbrand -
Mike Rapoport