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.
