Re: [PATCH v5 1/5] mm,memory_hotplug: Allocate memmap from the added memory range

From: Michal Hocko <mhocko@suse.com>
To: Oscar Salvador <osalvador@suse.de>
Cc: David Hildenbrand <david@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Pavel Tatashin <pasha.tatashin@soleen.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/5] mm,memory_hotplug: Allocate memmap from the added memory range
Date: Thu, 25 Mar 2021 15:40:04 +0100	[thread overview]
Message-ID: <YFygxF5Rx0ESCfKB@dhcp22.suse.cz> (raw)
In-Reply-To: <YFyX8jRWqfqCoGo/@localhost.localdomain>

On Thu 25-03-21 15:02:26, Oscar Salvador wrote:
> On Thu, Mar 25, 2021 at 01:26:34PM +0100, Michal Hocko wrote:
> > Yeah, David has raised the contiguous flag for zone already. And to be
> > completely honest I fail to see why we should shape a design based on an
> > optimization. If anything we can teach set_zone_contiguous to simply
> > ignore zone affiliation of vmemmap pages. I would be really curious if
> > that would pose any harm to the compaction code as they are reserved and
> > compaction should simply skip them.
> 
> No, compaction code is clever enough to skip over those pages as it
> already does for any Reserved page.
> My comment was more towards having the zone contiguous.
> 
> I know it is an optimization, but
> 
>  commit 7cf91a98e607c2f935dbcc177d70011e95b8faff
>  Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>  Date:   Tue Mar 15 14:57:51 2016 -0700
>  
>  mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous
> 
> talks about 30% of improvment. I am not sure if those numbers would
> still hold nowawadys, but it feels wrong to drop it to the ground when
> we can do better there, and IMHO, it does not overly complicate things.

Again, do not shape design around an optimization. If this turns out a
real problem then it can be handled on top.

> > THere is nothing like a proper zone.
> 
> I guess not, but for me it makes sense that vmemmap pages stay within
> the same zone as the pages they describe.

This is not the case for normal hotplug so why this should be any
different.

> Of course, this is a matter of opinions/taste.
> 
> > Not sure what you are referring to but if you have prior to f1dd2cd13c4b
> > ("mm, memory_hotplug: do not associate hotadded memory to zones until
> > online") then this was entirely a different story. Users do care where
> > they memory goes because that depends on the usecase but do they care
> > about vmemmap?
> 
> As I said, that is not what I am worried about.
> Users do not really care where those pages end up, that is transparent
> to them (wrt. vmemmap pages), but we (internally) kind of do.
> 
> So, as I said, I see advantatges of using your way, but I see downsides
> as:
> 
> - I would like to consider zone, and for that we would have to pull
>   some of the functions that check for the zone at an aearly stage, and
>   the mere thought sounds ugly.

This is impossible and whatever kind of heuristic you come up with might
be wrong.

> - Section containing vmemmap can remain offline and would have to come
>   up to sort that out

Yes, this is a problem indeed and as I've said in other email this would
be a problem for your initial implementation as well if the memory block
is still offline. I suspect we need to treat these Vmemmap pages as
online (via pfn_to_online_page).
-- 
Michal Hocko
SUSE Labs