On 24.03.21 14:40, Michal Hocko wrote:

On Wed 24-03-21 14:13:31, David Hildenbrand wrote:

quoted

On 24.03.21 13:37, Michal Hocko wrote:

quoted

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

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.

I don't think that's an issue. Just another special case to keep things 
simple. (and not completely fragment zones, mess with zone shrinking etc.)

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.

Exactly, as you said, the zone doesn't really matter - thus, this patch 
just handles it as simple as possible: keep them in the same zone as the 
hole memory block. No fragmented zones. no special casing. simple.

Details are actually pretty much all at a single place when 
onlining/offlining().

quoted

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.

Indeed.

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.

It's an offset in the beginning, so it's a special case. And the 
question is if there is a real benefit in handling it differently, for 
example, messing with online sections, messing with zones .. I am not 
convinced that the added complexity gives us a real benefit. But I shall 
be taught otherwise.


BTW: I once thought about having 
online_memory_block(block)/offline_memory_block(block) as separate 
functions instead of having pretty generic (error prone?) 
online_pages()/offline_pages(). Then, these details would just go in 
there and memory blocks + online/offline logic would simply be 
self-contained.

-- 
Thanks,

David / dhildenb