Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 26 May 2020 14:32:44 +0300
From:   Mike Rapoport <rppt@linux.ibm.com>
To:     Baoquan He <bhe@redhat.com>
Cc:     mgorman@suse.de, david@redhat.com, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, akpm@linux-foundation.org, cai@lca.pw,
        mhocko@kernel.org
Subject: Re: [PATCH] mm/compaction: Fix the incorrect hole in
 fast_isolate_freepages()
Message-ID: <20200526113244.GH13212@linux.ibm.com>
References: <20200521014407.29690-1-bhe@redhat.com>
 <20200521092612.GP1059226@linux.ibm.com>
 <20200521155225.GA20045@MiWiFi-R3L-srv>
 <20200521171836.GU1059226@linux.ibm.com>
 <20200522070114.GE26955@MiWiFi-R3L-srv>
 <20200522072524.GF26955@MiWiFi-R3L-srv>
 <20200522142053.GW1059226@linux.ibm.com>
 <20200526084543.GG26955@MiWiFi-R3L-srv>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200526084543.GG26955@MiWiFi-R3L-srv>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Baoquan,

On Tue, May 26, 2020 at 04:45:43PM +0800, Baoquan He wrote:
> On 05/22/20 at 05:20pm, Mike Rapoport wrote:
> > Hello Baoquan,
> > 
> > On Fri, May 22, 2020 at 03:25:24PM +0800, Baoquan He wrote:
> > > On 05/22/20 at 03:01pm, Baoquan He wrote:
> > > > 
> > > > So let's add these unavailable ranges into memblock and reserve them
> > > > in init_unavailable_range() instead. With this change, they will be added
> > > > into appropriate node and zone in memmap_init(), and initialized in
> > > > reserve_bootmem_region() just like any other memblock reserved regions.
> > > 
> > > Seems this is not right. They can't get nid in init_unavailable_range().
> > > Adding e820 ranges may let them get nid. But the hole range won't be
> > > added to memblock, and still has the issue.
> > > 
> > > Nack this one for now, still considering.
> > 
> > Why won't we add  the e820 reserved ranges to memblock.memory during
> > early boot as I suggested?
> > 
> > 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;
> > 
> > The setting of node later  in numa_init() will assign the proper node
> > for these regions as it does for the usable memory.
> 
> Yes, if it's only related to e820 reserved region, this truly works.
> 
> However, it also has ACPI table regions. That's why I changed to call
> the problematic area as firmware reserved ranges later.
> 
> Bisides, you can see below line, there's another reserved region which only
> occupies one page in one memory seciton. If adding to memblock.memory, we also
> will build struct mem_section and the relevant struct pages for the whole
> section. And then the holes around that page will be added and initialized in
> init_unavailable_mem(). numa_init() will assign proper node for memblock.memory
> and memblock.reserved, but won't assign proper node for the holes.
> 
> ~~~
> [    0.000000] BIOS-e820: [mem 0x00000000fed80000-0x00000000fed80fff] reserved
> ~~~
> 
> So I still think we should not add firmware reserved range into
> memblock for fixing this issue.
> 
> And, the fix in the original patch seems necessary. You can see in
> compaction code, the migration source is chosen from LRU pages or
> movable pages, the migration target has to be got from Buddy. However,
> only the min_pfn in fast_isolate_freepages(), it's calculated by
> distance between cc->free_pfn - cc->migrate_pfn, we can't guarantee it's
> safe, then use it as the target to handle.

I do not object to your original fix with careful check for pfn validity.

But I still think that the memory reserved by the firmware is still
memory and it should be added to memblock.memory. This way the memory
map will be properly initialized from the very beginning and we won't
need init_unavailable_mem() and alike workarounds and. Obviously, the patch
above is not enough, but it's a small step in this direction.

I believe that improving the early memory initialization would make many
things simpler and more robust, but that's a different story :)

-- 
Sincerely yours,
Mike.