From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8B7A228506B for ; Tue, 2 Dec 2025 10:57:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764673029; cv=none; b=XVVtENRy3wptlty+DP9GUQzx8ijH3SxNaTNvHsVszDhNTCwR9IpE5S1/3/PiktdLN7BEkbcKfJcMDWqvXde2Ek6FbMYBsDSlrAfXvWs3CI46bjOb7hHFDGkPQLWB01iZ58XpinPOwzSCTu/CMhE/KRSChASzIf4N1Q1WzOKBQLc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764673029; c=relaxed/simple; bh=PYrUdY1NU4nISxSv29OFIk58y1zwHcGNkFsZHi3SAdk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=qwWUlU7Jmy2NZJOURVFPhUvkayh/rBUxuTyy6zqHIwdpxR41MPeg21EfQmNZAHKb1bPbEImo/f3jVPN3K3Nk7747BwEloGtmbTAzyIwyDlQRJ4EOUEGDLP0avx622eLVwOdikSfllxT9oKnkwRMpC2syzgJ409hjscBmOgY15Zk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=jLYtwR9p; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="jLYtwR9p" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 09421C4CEF1; Tue, 2 Dec 2025 10:57:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1764673028; bh=PYrUdY1NU4nISxSv29OFIk58y1zwHcGNkFsZHi3SAdk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jLYtwR9pvYhlPyqJ45JOeG/uOScsGPl42Udz10SQbgkMMZxiO7JBA0cFpkkTMpW+2 yv/PSZw/yfPYyLnNbR/yEjIqBWzqwMB/wuQsvGoT84yfSWFu7koDlaxMVkZpm3RCGB ULMih6gYRNHkqClQ48qgkIe1UWZGc+vswMy6GOP+XOc63wl/bZpaDpmgF+n7yfjlh/ HZx7fn1oVDB0o1gF/eNPOANkmYIPfktqtNhOTvo9QSNAaAha5KyrGfiGycJdvwY7fC cEKP51K0ldlMSTUz8WcRmxIkBgYfGESDwBIjH5J7I7zNZKE+dtGBfXkknr+LKUrjfQ q1nmR8jhtI6wg== Date: Tue, 2 Dec 2025 12:57:00 +0200 From: Mike Rapoport To: "David Hildenbrand (Red Hat)" Cc: Tianyou Li , Oscar Salvador , Wei Yang , linux-mm@kvack.org, Yong Hu , Nanhai Zou , Yuan Liu , Tim Chen , Qiuxu Zhuo , Yu C Chen , Pan Deng , Chen Zhang , linux-kernel@vger.kernel.org Subject: Re: [PATCH v4] mm/memory hotplug/unplug: Optimize zone->contiguous update when changes pfn range Message-ID: References: <20251201132216.1636924-1-tianyou.li@intel.com> <7633c77b-44eb-41f0-9c3a-1e5034b594e3@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7633c77b-44eb-41f0-9c3a-1e5034b594e3@kernel.org> 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.