Re: [PATCH v9 3/8] mm,memory_hotplug: Factor out adjusting present pages into adjust_present_page_count()

[Michal Hocko]

On Wed 21-04-21 10:00:36, Oscar Salvador wrote:
> On Tue, Apr 20, 2021 at 11:45:55AM +0200, Michal Hocko wrote:
> > On Fri 16-04-21 13:24:06, Oscar Salvador wrote:
> > > From: David Hildenbrand <david@xxxxxxxxxx>
> > > 
> > > Let's have a single place (inspired by adjust_managed_page_count()) where
> > > we adjust present pages.
> > > In contrast to adjust_managed_page_count(), only memory onlining/offlining
> > > is allowed to modify the number of present pages.
> > > 
> > > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> > > Signed-off-by: Oscar Salvador <osalvador@xxxxxxx>
> > > Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>
> > 
> > Not sure self review counts ;)
> 
> Uhm, the original author is David, I just added my signed-off-by as a deliverer.
> I thought that in that case was ok to stick my Reviewed-by.
> Or maybe my signed-off-by carries that implicitly.

Yeah I do expect that one should review own changes but this is not
really anything to lose sleep over.

> > Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> > 
> > Btw. I strongly suspect the resize lock is quite pointless here.
> > Something for a follow up patch.
> 
> What makes you think that?

         * Write access to present_pages at runtime should be protected by
         * mem_hotplug_begin/end(). Any reader who can't tolerant drift of
         * present_pages should get_online_mems() to get a stable value.

> I have been thinking about this, let us ignore this patch for a moment.
> 
> If I poked the code correctly, node_size_lock is taken in:
> 
> remove_pfn_range_from_zone()
> move_pfn_range_to_zone()
> 
> both of them handling {zone,node}->spanned_pages
> 
> Then we take it in {offline,online}_pages() for {zone,node}->present_pages.
> 
> The other places where we take it are __init functions, so not of interest.
> 
> Given that {offline,online}_pages() is serialized by the memory_hotplug lock,
> I would say that {node,zone}->{spanned,present}_pages is, at any time, stable?
> So, no need for the lock even without considering this patch?

Yes. The resize lock is really only relevant to the parallel struct page
initialization during early boot. The hotplug usage seems just a left
over from the past or maybe it has never been really relevant in that
context.

> Now, getting back to this patch.
> adjust_present_page_count() will be called from memory_block_online(), which
> is not holding the memory_hotplug lock yet.
> But, we only fiddle with present pages out of {online,offline}_pages() if
> we have vmemmap pages, and since that operates on the same memory block,
> its lock should serialize that.

Memory hotplug is always synchronized on the device level.
-- 
Michal Hocko
SUSE Labs