Re: [PATCH] mm, hotplug: protect nr_zones with pgdat_resize_lock()

[Wei Yang <richard.weiyang@gmail.com>]

On Wed, Nov 21, 2018 at 3:15 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 21-11-18 02:52:31, Wei Yang wrote:
> > On Tue, Nov 20, 2018 at 08:58:11AM +0100, osalvador@suse.de wrote:
> > >> On the other hand I would like to see the global lock to go away because
> > >> it causes scalability issues and I would like to change it to a range
> > >> lock. This would make this race possible.
> > >>
> > >> That being said this is more of a preparatory work than a fix. One could
> > >> argue that pgdat resize lock is abused here but I am not convinced a
> > >> dedicated lock is much better. We do take this lock already and spanning
> > >> its scope seems reasonable. An update to the documentation is due.
> > >
> > >Would not make more sense to move it within the pgdat lock
> > >in move_pfn_range_to_zone?
> > >The call from free_area_init_core is safe as we are single-thread there.
> > >
> >
> > Agree. This would be better.
> >
> > >And if we want to move towards a range locking, I even think it would be more
> > >consistent if we move it within the zone's span lock (which is already
> > >wrapped with a pgdat lock).
> > >
> >
> > I lost a little here, just want to confirm with you.
> >
> > Instead of call pgdat_resize_lock() around init_currently_empty_zone()
> > in move_pfn_range_to_zone(), we move init_currently_empty_zone() before
> > resize_zone_range()?
> >
> > This sounds reasonable.
>
> Btw. resolving the existing TODO would be nice as well, now that you are
> looking that direction...
>

Michal,

I took a look at commit f1dd2cd13c4b ("mm, memory_hotplug: do not
associate hotadded memory to zones until online"), and try to understand
this TODO.

The reason to acquire these lock is before this commit, the memory is
associated with zone at physical adding phase, while after this commit,
this is delayed to logical online stage.

But I get some difficulty to understand this TODO. You want to get rid of
these lock? While these locks seem necessary to protect those data of
pgdat/zone. Would you mind sharing more on this statement?