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

[Oscar Salvador]

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@xxxxxxx>
 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.

> 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.
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.
- Section containing vmemmap can remain offline and would have to come
  up to sort that out

Of course, the big advantatge is that we do things at its right time.

Now, doing things as David and I suggested (that is, create a helper to
do the accounting/initialization of vmemmap at online stage but without
populing online/offline_pages()) has the downside of messing with
different concepts that have different lifecycle, but I see an enormous
advantatge and that is it keeps things fairly simple.


I do not want to be seen that I am pushing back just for the sake of
doing so, and I am more than open to explore other means.
Actually, the code has evolved and re-shaped quite a lot since its beginning,
so nothing really speaks against it, but I think the idea of the helper
is less troublesome and simple.

-- 
Oscar Salvador
SUSE L3