Re: [PATCH v2 1/3] mm,page_alloc: Use {get,put}_online_mems() to get stable zone's values

[David Hildenbrand]

On 07.06.21 09:52, Oscar Salvador wrote:
> On Fri, Jun 04, 2021 at 09:41:45AM +0200, Oscar Salvador wrote:
> > On Thu, Jun 03, 2021 at 02:45:13PM +0200, Michal Hocko wrote:
> > > I believe we need to define the purpose of the locking first. The
> >
> > If you ask me, this locking would be meant to make sure zone's zone_start_pfn
> > or spanned_pages do not change under us, in case we __need__ the value to be
> > stable.
> >
> > > existing locking doesn't serve much purpose, does it? The state might
> >
> > Well, half-way. Currently, the locking is taken in write mode whenever
> > the zone is expanded or shrinked, and in read mode when called from
> > bad_range()->page_outside_zone_boundaries() (only on VM_DEBUG).
> >
> > But as you pointed out, such state might change right after the locking is
> > released and all the work would be for nothing.
> > So indeed, the __whole__ operation should be envolved by the lock in the caller
> > The way that stands right now is not optimal.
> >
> > > change right after the lock is released and the caller cannot really
> > > rely on the result. So aside of the current implementation, I would
> > > argue that any locking has to be be done on the caller layer.
> > >
> > > But the primary question is whether anybody actually cares about
> > > potential races in the first place.
> >
> > I have been checking move_freepages_block() and alloc_contig_pages(), which
> > are two of the functions that call zone_spans_pfn().
> >
> > move_freepages_block() uses it in a way to align the given pfn to pageblock
> > top and bottom, and then check that aligned pfns are still within the same zone.
> >  From a memory-hotplug perspective that's ok as we know that we are offlining
> > PAGES_PER_SECTION (which implies whole pageblocks).
> >
> > alloc_contig_pages() (used by the hugetlb gigantic allocator) runs through a
> > node's zonelist and checks whether zone->zone_start_pfn + nr_pages stays within
> > the same zone.
> > IMHO, the race with zone_spans_last_pfn() vs mem-hotplug would not be that bad,
> > as it will be caught afters by e.g: __alloc_contig_pages when pages cannot be
> > isolated because they are offline etc.
> >
> > So, I would say we do not really need the lock, but I might be missing something.
> > But if we chose to care about this, then the locking should be done right, not
> > half-way as it is right now.
>
>
> Any thoughts on this? :-)

I'd like to point out that I think the seqlock is not in place to 
synchronize with actual growing/shrinking but to get consistent zone 
ranges -- like using atomics, but we have two inter-dependent values here.

If you obtain the zone ranges that way and properly use 
pfn_to_online_page(), there is hardly something that can go wrong in 
practice. If the zone grew in the meantime, most probably you can just 
live with not processing that part for now. If the zone shrunk in the 
meantime, pfn_to_online_page() will make you skip that part (it was 
offlined either way, so you most probably don't really care about that 
part).

[pfn_to_online_page() is racy as well, but the race window is very small 
and we never saw a problem in practice really]

Without the seqlock, you might just get a garbage zone range and have 
either false/positive negatives when just testing for a simple range not 
in an hot(un)plugged range [which is the usual case when talking about 
compaction etc.].

--
Thanks,

David / dhildenb