On 24.03.21 13:37, Michal Hocko wrote:

On Wed 24-03-21 13:23:47, David Hildenbrand wrote:

quoted

On 24.03.21 13:10, Michal Hocko wrote:

quoted

On Wed 24-03-21 13:03:29, Michal Hocko wrote:

quoted

On Wed 24-03-21 11:12:59, Oscar Salvador wrote:

[...]

an additional remark

quoted

quoted

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

quoted

@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 :) ), 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).

Is that a sufficient concern to nack the whole thing? No, I do not think
so. But I do not see any particular rush to have this work needs to be
merged ASAP.

Sure, there is no need to rush (not that I suggested that).

-- 
Thanks,

David / dhildenb