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

[Michal Hocko]

On Wed 24-03-21 14:13:31, David Hildenbrand wrote:
> On 24.03.21 13:37, Michal Hocko wrote:
> > On Wed 24-03-21 13:23:47, David Hildenbrand wrote:
> > > On 24.03.21 13:10, Michal Hocko wrote:
> > > > On Wed 24-03-21 13:03:29, Michal Hocko wrote:
> > > > > On Wed 24-03-21 11:12:59, Oscar Salvador wrote:
> > > > [...]
> > > > 
> > > > an additional remark
> > > > 
> > > > > > - online_pages()->move_pfn_range_to_zone(): Accounts for node/zone's spanned pages
> > > > > > - online_pages()->zone->present_pages += nr_pages;
> > > > 
> > > > I am pretty sure you shouldn't account vmmemmap pages to the target zone
> > > > in some cases - e.g. vmemmap cannot be part of the movable zone, can it?
> > > > So this would be yet another special casing. This patch has got it wrong
> > > > unless I have missed some special casing.
> > > > 
> > > 
> > > It's a bit unfortunate that we have to discuss the very basic design
> > > decisions again.
> > 
> > It would be great to have those basic design decisions layed out in the
> > changelog.
> > 
> > > @Oscar, maybe you can share the links where we discussed all this and add
> > > some of it to the patch description.
> > > 
> > > I think what we have right here is good enough for an initial version, from
> > > where on we can improve things without having to modify calling code.
> > 
> > I have to say I really dislike vmemmap proliferation into
> > {on,off}lining. It just doesn't belong there from a layering POV. All
> > this code should care about is to hand over pages to the allocator and
> > make them visible.
> 
> Well, someone has to initialize the vmemmap of the vmemmap pages ( which is
> itself :) ),

Yeah, and I would expect this to be done when the vmemmap space is
reserved. This is at the hotadd time and we do not know the zone but
that shouldn't really matter because their zone can be quite arbitrary
kernel zone. As mentioned previously I do not think associating those
with zone movable is a good idea as they are fundamentally not movable.
It is likely that the zone doesn't really matter for these pages anyway
and the only think we do care about is that they are not poisoned and
there is at least something but again it would be much better to have a
single place where all those details are done (including accounting)
rather than trying to wrap head around different pfns when onlining
pages and grow potential and suble bugs there.

> and as the vemmap does not span complete sections things can
> get very weird as we can only set whole sections online (there was more to
> that, I think it's buried in previous discussions).

Yes the section can only online as whole. This is an important "detail"
and it would deserve some more clarification in the changelog as well.
You have invested quite some energy into code consolidation and checks
to make sure that hotplugged code doesn't have holes and this work bends
those rules. vmemmap is effectivelly a hole in a memblock/section. I
think we should re-evaluate some of those constrains rather than try to
work them around somehow.
-- 
Michal Hocko
SUSE Labs