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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <49bda900-e79f-4d43-8ea8-46e7d9f78c2d@intel.com>
Date: Mon, 8 Dec 2025 23:05:49 +0800
From: "Li, Tianyou" <tianyou.li@...el.com>
To: "David Hildenbrand (Red Hat)" <david@...nel.org>, Oscar Salvador
	<osalvador@...e.de>, Mike Rapoport <rppt@...nel.org>, Wei Yang
	<richard.weiyang@...il.com>
CC: <linux-mm@...ck.org>, Yong Hu <yong.hu@...el.com>, Nanhai Zou
	<nanhai.zou@...el.com>, Yuan Liu <yuan1.liu@...el.com>, Tim Chen
	<tim.c.chen@...ux.intel.com>, Qiuxu Zhuo <qiuxu.zhuo@...el.com>, Yu C Chen
	<yu.c.chen@...el.com>, Pan Deng <pan.deng@...el.com>, Chen Zhang
	<zhangchen.kidd@...com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] mm/memory hotplug/unplug: Optimize zone->contiguous
 update when changes pfn range

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.
>
>
>
>