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

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

Sorry for the delayed. We were working on a suspected issue and now the 
patch v5 is out. Thanks for your review in advance.


Regards,

Tianyou


On 12/2/2025 6:41 PM, Li, Tianyou wrote:
>
> 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.
>
>
>
>