Re: [PATCH v2] mm/page_alloc: Add lockdep assertion for pageblock type change

[Brendan Jackman <jackmanb@google.com>]

On Mon, Mar 03, 2025 at 02:11:23PM +0100, David Hildenbrand wrote:
> On 03.03.25 13:13, Brendan Jackman wrote:
> > Since the migratetype hygiene patches [0], the locking here is
> > a bit more formalised.
> > 
> > For other stuff, it's pretty obvious that it would be protected by the
> > zone lock. But it didn't seem totally self-evident that it should
> > protect the pageblock type. So it seems particularly helpful to have it
> > written in the code.
> 
> [...]
> 
> > +
> >   u64 max_mem_size = U64_MAX;
> >   /* add this memory to iomem resource */
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 579789600a3c7bfb7b0d847d51af702a9d4b139a..1ed21179676d05c66f77f9dbebf88e36bbe402e9 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -417,6 +417,10 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
> >   void set_pageblock_migratetype(struct page *page, int migratetype)
> >   {
> > +	lockdep_assert_once(system_state == SYSTEM_BOOTING ||
> > +		in_mem_hotplug() ||
> > +		lockdep_is_held(&page_zone(page)->lock));
> > +
> 
> I assume the call chain on the memory hotplug path is mostly
> 
> move_pfn_range_to_zone()->memmap_init_range()->set_pageblock_migratetype()
> 
> either when onlining a memory block, or from pagemap_range() while holding
> the hotplug lock.
> 
> But there is also the memmap_init_zone_device()->memmap_init_compound()->__init_zone_device_page()->set_pageblock_migratetype()
> one, called from pagemap_range() *without* holding the hotplug lock, and you
> assertion would be missing that.
> 
> I'm not too happy about that assertion in general.

Hmm, thanks for pointing that out. 

I guess if we really wanted the assertion the approach would be to
replace in_mem_hotplug() with some more fine-grained logic about the
state of the pageblock? But that seems like it would require rework
that isn't really justified.

So yeah I guess this synchronization isn't as ready as I thought for
such a straightforwad "you need one of these locks here" assertion.

My ulterior motive here is the series I'm working on where the
pageblock records the ASI mapping status, so maybe I can find a weaker
assertion that at least helps with that more specific logic.