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

[Brendan Jackman <jackmanb@google.com>]

On Mon, Mar 03, 2025 at 06:06:12PM +0100, David Hildenbrand wrote:
> > > But I am not sure why memmap_init_zone_device() would have to set the
> > > migratetype at all? Because migratetype is a buddy concept, and
> > > ZONE_DEVICE does not interact with the buddy to that degree.
> > > 
> > > The comment in __init_zone_device_page states:
> > > 
> > > "Mark the block movable so that blocks are reserved for movable at
> > > startup. This will force kernel allocations to reserve their blocks
> > > rather than leaking throughout the address space during boot when
> > > many long-lived kernel allocations are made."
> > 
> > Uh, yeah I was pretty mystified by that. It would certainly be nice if
> > we can just get rid of this modification path.
> > 
> > > But that just dates back to 966cf44f637e where we copy-pasted that code.
> > > 
> > > So I wonder if we could just
> > > 
> > > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > > index 57933683ed0d1..b95f545846e6e 100644
> > > --- a/mm/mm_init.c
> > > +++ b/mm/mm_init.c
> > > @@ -1002,19 +1002,11 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
> > >          page->zone_device_data = NULL;
> > >          /*
> > > -        * Mark the block movable so that blocks are reserved for
> > > -        * movable at startup. This will force kernel allocations
> > > -        * to reserve their blocks rather than leaking throughout
> > > -        * the address space during boot when many long-lived
> > > -        * kernel allocations are made.
> > > -        *
> > > -        * Please note that MEMINIT_HOTPLUG path doesn't clear memmap
> > > -        * because this is done early in section_activate()
> > > +        * Note that we leave pageblock migratetypes uninitialized, because
> > > +        * they don't apply to ZONE_DEVICE.
> > >           */
> > > -       if (pageblock_aligned(pfn)) {
> > > -               set_pageblock_migratetype(page, MIGRATE_MOVABLE);
> > > +       if (pageblock_aligned(pfn))
> > >                  cond_resched();
> > > -       }
> > >          /*
> > >           * ZONE_DEVICE pages other than MEMORY_TYPE_GENERIC are released
> > 
> > memory-model.rst says:
> > 
> > > Since the
> > > page reference count never drops below 1 the page is never tracked as
> > > free memory and the page's `struct list_head lru` space is repurposed
> > > for back referencing to the host device / driver that mapped the memory.
> 
> That comment will be stale soon. In general, ZONE_DEVICE refcounts can drop
> to 0, but they will never go to the buddy, but will get intercepted on the
> freeing path.
> 
> > 
> > And this code seems to assume that the whole pageblock is part of the
> > ZONE_DEVICE dance, it would certainly make sense to me...
> 
> Sorry, I didn't get your final conclusion: do you thing we don't have to
> initialize the migratetype, or do you think there is reason to still do it?

Sorry yeah, I was concluding that I don't see any reason to set the
migratetype here. But, given I didn't even notice this code path until
your review here I am not feeling too confident about changing it.

I can try to stare it some more and hopefully some courage will build
up! I probably also need to find a way to run a system that uses
ZONE_DEVICE for my local testing...