Date:   Thu, 21 May 2020 23:52:25 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Mike Rapoport <rppt@...ux.ibm.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        akpm@...ux-foundation.org, cai@....pw, mgorman@...e.de,
        mhocko@...nel.org
Subject: Re: [PATCH] mm/compaction: Fix the incorrect hole in
 fast_isolate_freepages()

On 05/21/20 at 12:26pm, Mike Rapoport wrote:
> > For this kind of e820 reserved range, it won't be added to memblock allocator.
> > However, init_unavailable_mem() will initialize to add them into node 0,
> > zone 0. Before that commit, later, memmap_init() will add e820 reserved
> > ranges into the zone where they are contained, because it can pass
> > the checking of early_pfn_valid() and early_pfn_in_nid(). In this case,
> > the e820 reserved range where fault page 0x2a800000 is located is added
> > into DMA32 zone. After that commit, the e820 reserved rgions are kept
> > in node 0, zone 0, since we iterate over memblock regions to iniatialize
> > in memmap_init() instead, their node and zone won't be changed.
> 
> I wonder why woudn't we add the reserved memory to memblock from the
> very beginning...
> I've tried to undestand why this was not done, but I couldn't find the
> reasoning behind that.

I have added some explanation when reply to Mel. Please check that in
that thread.

As I said, the unavailable range includes firmware reserved ranges, and
holes inside one boot memory section, if that boot memory section haves
useable memory range, and firmware reserved ranges, and holes. Adding
them all into memblock seems a little unreasonable, since they are never
used by system in memblock, buddy or high level memory allocator. But I
can see that adding them into memblock may have the same effect as the
old code which is beofre your your patchset applied. Let's see if Mel or
other people have some saying. I pesonally would not suggest doing it
like this though.

> 
> Can you please try the below patch and see if it helps or, on the
> contrary, breaks anything else :)
> 
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index c5399e80c59c..b0940c618ed9 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -1301,8 +1301,11 @@ void __init e820__memblock_setup(void)
>  		if (end != (resource_size_t)end)
>  			continue;
>  
> -		if (entry->type == E820_TYPE_SOFT_RESERVED)
> +		if (entry->type == E820_TYPE_SOFT_RESERVED ||
> +		    entry->type == E820_TYPE_RESERVED) {
> +			memblock_add(entry->addr, entry->size);
>  			memblock_reserve(entry->addr, entry->size);
> +		}
>  
>  		if (entry->type != E820_TYPE_RAM && entry->type != E820_TYPE_RESERVED_KERN)
>  			continue;
> 
> > Now, fast_isolate_freepages() will use min mark directly as the migration
> > target if no page is found from buddy. However, the min mark is not checked
> > carefully, it could be in e820 reserved range, and trigger the
> > VM_BUG_ON_PAGE(!zone_spans_pfn(page_zone(page), pfn)) when try to mark it
> > as skip.
> > 
> > Here, let's call pageblock_pfn_to_page() to get page of min_pfn, since it
> > will do careful checks and return NULL if the pfn is not qualified.
> > 
> > [    0.000000] BIOS-provided physical RAM map:
> > [    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000008bfff] usable
> > [    0.000000] BIOS-e820: [mem 0x000000000008c000-0x000000000009ffff] reserved
> > [    0.000000] BIOS-e820: [mem 0x00000000000e0000-0x00000000000fffff] reserved
> > [    0.000000] BIOS-e820: [mem 0x0000000000100000-0x0000000028328fff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000028329000-0x0000000028568fff] reserved
> > [    0.000000] BIOS-e820: [mem 0x0000000028569000-0x0000000028d85fff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000028d86000-0x0000000028ee5fff] reserved
> > [    0.000000] BIOS-e820: [mem 0x0000000028ee6000-0x0000000029a04fff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000029a05000-0x0000000029a08fff] reserved
> > [    0.000000] BIOS-e820: [mem 0x0000000029a09000-0x0000000029ee4fff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000029ee5000-0x0000000029ef2fff] ACPI data
> > [    0.000000] BIOS-e820: [mem 0x0000000029ef3000-0x0000000029f22fff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000029f23000-0x0000000029f23fff] ACPI data
> > [    0.000000] BIOS-e820: [mem 0x0000000029f24000-0x0000000029f24fff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000029f25000-0x0000000029f28fff] ACPI data
> > [    0.000000] BIOS-e820: [mem 0x0000000029f29000-0x0000000029f29fff] ACPI NVS
> > [    0.000000] BIOS-e820: [mem 0x0000000029f2a000-0x0000000029f2bfff] ACPI data
> > [    0.000000] BIOS-e820: [mem 0x0000000029f2c000-0x0000000029f2cfff] reserved
> > [    0.000000] BIOS-e820: [mem 0x0000000029f2d000-0x0000000029f2ffff] ACPI data
> > [    0.000000] BIOS-e820: [mem 0x0000000029f30000-0x0000000029ffdfff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000029ffe000-0x000000002a80afff] reserved
> > [    0.000000] BIOS-e820: [mem 0x000000002a80b000-0x000000002a80efff] ACPI NVS
> > [    0.000000] BIOS-e820: [mem 0x000000002a80f000-0x000000002a81ffff] reserved
> > [    0.000000] BIOS-e820: [mem 0x000000002a820000-0x000000002a822fff] ACPI NVS
> > [    0.000000] BIOS-e820: [mem 0x000000002a823000-0x0000000033a22fff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000033a23000-0x0000000033a32fff] reserved
> > [    0.000000] BIOS-e820: [mem 0x0000000033a33000-0x0000000033a42fff] ACPI NVS
> > [    0.000000] BIOS-e820: [mem 0x0000000033a43000-0x0000000033a52fff] ACPI data
> > [    0.000000] BIOS-e820: [mem 0x0000000033a53000-0x0000000077ffffff] usable
> > [    0.000000] BIOS-e820: [mem 0x0000000078000000-0x000000008fffffff] reserved
> > [    0.000000] BIOS-e820: [mem 0x00000000fed80000-0x00000000fed80fff] reserved
> > [    0.000000] BIOS-e820: [mem 0x0000000100000000-0x000000087effffff] usable
> > [    0.000000] BIOS-e820: [mem 0x000000087f000000-0x000000087fffffff] reserved
> > 
> > Reported-by: Qian Cai <cai@....pw>
> > Signed-off-by: Baoquan He <bhe@...hat.com>
> > ---
> >  mm/compaction.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 67fd317f78db..9ce4cff4d407 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -1418,7 +1418,9 @@ fast_isolate_freepages(struct compact_control *cc)
> >  				cc->free_pfn = highest;
> >  			} else {
> >  				if (cc->direct_compaction && pfn_valid(min_pfn)) {
> > -					page = pfn_to_page(min_pfn);
> > +					page = pageblock_pfn_to_page(min_pfn,
> > +						pageblock_end_pfn(min_pfn),
> > +						cc->zone);
> >  					cc->free_pfn = min_pfn;
> >  				}
> >  			}
> > -- 
> > 2.17.2
> > 
> 
> -- 
> Sincerely yours,
> Mike.
> 

Powered by blists - more mailing lists