| 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
| ||
|
Message-ID: <9102cb70-e573-42d8-82c3-39d2af009e63@intel.com>
Date: Wed, 3 Dec 2025 17:35:47 +0800
From: "Li, Tianyou" <tianyou.li@...el.com>
To: Mike Rapoport <rppt@...nel.org>, "David Hildenbrand (Red Hat)"
<david@...nel.org>
CC: Oscar Salvador <osalvador@...e.de>, Wei Yang <richard.weiyang@...il.com>,
<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
On 12/2/2025 6:57 PM, Mike Rapoport wrote:
> On Mon, Dec 01, 2025 at 07:54:34PM +0100, David Hildenbrand (Red Hat) wrote:
>> On 12/1/25 14:22, Tianyou Li wrote:
> ...
>
>>> 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);
> +1
>
>>> +static enum zone_contiguous_state __meminit clear_zone_contiguous_for_growing(
>>> + 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;
>>> +
>>> + /*
>>> + * Given the moved pfn range's contiguous property is always true,
>>> + * under the conditional of empty zone, the contiguous property should
>>> + * be true.
>>> + */
>> I don't think that comment is required.
>>
>>> + if (zone_is_empty(zone))
>>> + result = CONTIGUOUS_DEFINITELY;
>>> +
>>> + /*
>>> + * If the moved pfn range does not intersect with the original zone span,
>>> + * the contiguous property is surely false.
>>> + */
>>> + else if (end_pfn < zone->zone_start_pfn || start_pfn > zone_end_pfn(zone))
>>> + result = CONTIGUOUS_DEFINITELY_NOT;
>>> +
>>> + /*
>>> + * If the moved pfn range is adjacent to the original zone span, given
>>> + * the moved pfn range's contiguous property is always true, the zone's
>>> + * contiguous property inherited from the original value.
>>> + */
>>> + else if (end_pfn == zone->zone_start_pfn || start_pfn == zone_end_pfn(zone))
>>> + result = zone->contiguous ?
>>> + CONTIGUOUS_DEFINITELY : CONTIGUOUS_DEFINITELY_NOT;
>>> +
>>> + /*
>>> + * If the original zone's hole larger than the moved pages in the range,
>>> + * the contiguous property is surely false.
>>> + */
>>> + else if (nr_pages < (zone->spanned_pages - zone->present_pages))
>>> + result = CONTIGUOUS_DEFINITELY_NOT;
>>> +
>> This is a bit unreadable :)
>>
>> if (zone_is_empty(zone)) {
>> result = CONTIGUOUS_DEFINITELY;
>> } else if (...) {
>> /* ... */
>> ...
>> } else if (...) {
>> ...
>> }
> If we update zone state outside this function it can be even simpler:
>
> if (zone_is_empty(zone))
> return CONTIGUOUS_DEFINITELY;
>
> if (nr_pages < (zone->spanned_pages - zone->present_pages))
> return CONTIGUOUS_DEFINITELY_NOT;
>
> etc.
>
Thanks Mike. It's doable, consider clear_zone_contiguous moved out.
>>> + clear_zone_contiguous(zone);
>>> + return result;
>>> +}
>>> +
>>> /*
>>> * Associate the pfn range with the given zone, initializing the memmaps
>>> * and resizing the pgdat/zone data to span the added pages. After this
>>> @@ -752,8 +821,8 @@ void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>>> {
>>> struct pglist_data *pgdat = zone->zone_pgdat;
>>> int nid = pgdat->node_id;
>>> -
>>> - clear_zone_contiguous(zone);
>>> + const enum zone_contiguous_state contiguous_state =
>>> + clear_zone_contiguous_for_growing(zone, start_pfn, nr_pages);
>>> if (zone_is_empty(zone))
>>> init_currently_empty_zone(zone, start_pfn, nr_pages);
>>> @@ -783,7 +852,7 @@ void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>>> MEMINIT_HOTPLUG, altmap, migratetype,
>>> isolate_pageblock);
>>> - set_zone_contiguous(zone);
>>> + set_zone_contiguous(zone, contiguous_state);
>>> }
>>> struct auto_movable_stats {
>>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>>> index 7712d887b696..06db3fcf7f95 100644
>>> --- a/mm/mm_init.c
>>> +++ b/mm/mm_init.c
>>> @@ -2263,26 +2263,34 @@ void __init init_cma_pageblock(struct page *page)
>>> }
>>> #endif
>>> -void set_zone_contiguous(struct zone *zone)
>>> +void set_zone_contiguous(struct zone *zone, enum zone_contiguous_state state)
>>> {
>>> unsigned long block_start_pfn = zone->zone_start_pfn;
>>> unsigned long block_end_pfn;
>>> - block_end_pfn = pageblock_end_pfn(block_start_pfn);
>>> - for (; block_start_pfn < zone_end_pfn(zone);
>>> - block_start_pfn = block_end_pfn,
>>> - block_end_pfn += pageblock_nr_pages) {
>>> + if (state == CONTIGUOUS_DEFINITELY) {
>>> + zone->contiguous = true;
>>> + return;
>>> + } else if (state == CONTIGUOUS_DEFINITELY_NOT) {
>>> + // zone contiguous has already cleared as false, just return.
> Please no C++ style comments.
Got it. Thanks.
>>> + return;
>>> + } else if (state == CONTIGUOUS_UNDETERMINED) {
>>> + block_end_pfn = pageblock_end_pfn(block_start_pfn);
>>> + for (; block_start_pfn < zone_end_pfn(zone);
>>> + block_start_pfn = block_end_pfn,
>>> + block_end_pfn += pageblock_nr_pages) {
>>> - block_end_pfn = min(block_end_pfn, zone_end_pfn(zone));
>>> + block_end_pfn = min(block_end_pfn, zone_end_pfn(zone));
>>> - if (!__pageblock_pfn_to_page(block_start_pfn,
>>> - block_end_pfn, zone))
>>> - return;
>>> - cond_resched();
>>> - }
>>> + if (!__pageblock_pfn_to_page(block_start_pfn,
>>> + block_end_pfn, zone))
>>> + return;
>>> + cond_resched();
>>> + }
>>> - /* We confirm that there is no hole */
>>> - zone->contiguous = true;
>>> + /* We confirm that there is no hole */
>>> + zone->contiguous = true;
>>> + }
>>> }
>>
>> switch (state) {
>> case CONTIGUOUS_DEFINITELY:
>> zone->contiguous = true;
>> return;
>> case CONTIGUOUS_DEFINITELY_NOT:
>> return;
>> default:
>> break;
>> }
>> ... unchanged logic.
> I was going to suggest rather to drop 'else'
>
> if (state == CONTIGUOUS_DEFINITELY) {
> zone->contiguous = true;
> return;
> }
>
> if (state == CONTIGUOUS_DEFINITELY_NOT)
> return;
>
> ... unchanged logic.
>
> but I don't feel strongly about it.
>
Got it. Thanks.
Powered by blists - more mailing lists