| 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: <aS7F_ORMLBcAqQTz@kernel.org>
Date: Tue, 2 Dec 2025 12:57:00 +0200
From: Mike Rapoport <rppt@...nel.org>
To: "David Hildenbrand (Red Hat)" <david@...nel.org>
Cc: Tianyou Li <tianyou.li@...el.com>, 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 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.
> > + 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.
> > + 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.
--
Sincerely yours,
Mike.
Powered by blists - more mailing lists