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