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

["Li, Tianyou" <tianyou.li@intel.com>]

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.