Re: [PATCH v4] mm/memory hotplug/unplug: Optimize zone->contiguous update when changes pfn range

[Li, Tianyou]

On 12/2/2025 6:24 PM, David Hildenbrand (Red Hat) wrote:
> > > > +};
> > >
> > > I don't like that the defines don't match the enum name (zone_c... vs.
> > > CONT... ).
> > >
> > > Essentially you want a "yes / no / maybe" tristate. I don't think we
> > > have an existing type for that, unfortunately.
> > >
> > > enum zone_contig_state {
> > >      ZONE_CONTIG_YES,
> > >      ZONE_CONTIG_NO,
> > >      ZONE_CONTIG_MAYBE,
> > > };
> > >
> > > Maybe someone reading along has a better idea.
> >
> > I agree it's better. Will wait for a day or two to make the change.
>
> Yes, good idea. No needs to rush at this point because the merge 
> window just opened up.

Got it. Allow me to take one more day then complete the patch v5 with 
sufficient testing.


> > > > +
> > > > +void set_zone_contiguous(struct zone *zone, enum
> > > > zone_contiguous_state state);
> > > >    bool pfn_range_intersects_zones(int nid, unsigned long start_pfn,
> > > >                   unsigned long nr_pages);
> > > >    diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > > index 0be83039c3b5..b74e558ce822 100644
> > > > --- a/mm/memory_hotplug.c
> > > > +++ b/mm/memory_hotplug.c
> > > > @@ -544,6 +544,32 @@ static void update_pgdat_span(struct pglist_data
> > > > *pgdat)
> > > >        pgdat->node_spanned_pages = node_end_pfn - node_start_pfn;
> > > >    }
> > > >    +static enum zone_contiguous_state __meminit
> > > > clear_zone_contiguous_for_shrinking(
> > > > +        struct zone *zone, unsigned long start_pfn, unsigned long
> > > > nr_pages)
> > > > +{
> > > > +    const unsigned long end_pfn = start_pfn + nr_pages;
> > > > +    enum zone_contiguous_state result = CONTIGUOUS_UNDETERMINED;
> > > > +
> > > > +    /*
> > > > +     * If the removed pfn range inside the original zone span, the
> > > > contiguous
> > > > +     * property is surely false.
> > > > +     */
> > > > +    if (start_pfn > zone->zone_start_pfn && end_pfn <
> > > > zone_end_pfn(zone))
> > > > +        result = CONTIGUOUS_DEFINITELY_NOT;
> > > > +
> > > > +    /*
> > > > +     * If the removed pfn range is at the beginning or end of the
> > > > +     * original zone span, the contiguous property is preserved when
> > > > +     * the original zone is contiguous.
> > > > +     */
> > > > +    else if (start_pfn == zone->zone_start_pfn || end_pfn ==
> > > > zone_end_pfn(zone))
> > > > +        result = zone->contiguous ?
> > > > +            CONTIGUOUS_DEFINITELY : CONTIGUOUS_UNDETERMINED;
> > > > +
> > >
> > > See my comment below on how to make this readable.
> > >
> > > > +    clear_zone_contiguous(zone);
> > > > +    return result;
> > > > +}
> > > > +
> > > >    void remove_pfn_range_from_zone(struct zone *zone,
> > > >                          unsigned long start_pfn,
> > > >                          unsigned long nr_pages)
> > > > @@ -551,6 +577,7 @@ void remove_pfn_range_from_zone(struct zone *zone,
> > > >        const unsigned long end_pfn = start_pfn + nr_pages;
> > > >        struct pglist_data *pgdat = zone->zone_pgdat;
> > > >        unsigned long pfn, cur_nr_pages;
> > > > +    enum zone_contiguous_state contiguous_state =
> > > > CONTIGUOUS_UNDETERMINED;
> > > >          /* Poison struct pages because they are now uninitialized
> > > > again. */
> > > >        for (pfn = start_pfn; pfn < end_pfn; pfn += cur_nr_pages) {
> > > > @@ -571,12 +598,13 @@ void remove_pfn_range_from_zone(struct zone 
> > > > *zone,
> > > >        if (zone_is_zone_device(zone))
> > > >            return;
> > > >    -    clear_zone_contiguous(zone);
> > > > +    contiguous_state = clear_zone_contiguous_for_shrinking(
> > > > +                zone, start_pfn, nr_pages);
> > >
> > > Reading this again, I wonder whether it would be nicer to have
> > > something like:
> > >
> > > new_contig_state = zone_contig_state_after_shrinking();
> > > clear_zone_contiguous(zone);
> > >
> > > or sth like that. Similar for the growing case.
> >
> > In both shrinking and growing case, separate the clear_zone_contiguous
> > from the logic of zone state check, right?
>
> Yes, I think that makes it look a bit nicer.

Thanks for the confirmation David. Noted and will do. Thanks.