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 09A8D2DAFC2 for ; Tue, 2 Dec 2025 10:25:02 +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=1764671103; cv=none; b=lA+2Q5TW0d1Zj4qXHZ6OKaH1tk3uboCpzdPFPDNSLpfo7q10kDJC+8+r2bgqckWtUTQS6okfFK6D8Qv/ScDPmsiaHLa6JovWFRY2xaYGIhm+baykzYD+Q5Y5+syup7Atmy4Ijh5jzNGe5k1SYwLxmJpXfpDz3SrIHjWXrDoFMNo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764671103; c=relaxed/simple; bh=McMIirB/3kBEvvRZPpsdhilMluP94hMyIECum5e3cYk=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=CnibcpaRLtFFGUT/z+h4bIhY3zW3JDUJrVkLxeoYxUlzuo3bZnvQY8qn8ZxVvJFR/y6Sv/iizaNaC3MH3wjV5t9/SgbRmfgFK3kcBQdBkq2LJ0SG9qHRhh70mnmStk6UGLSfsANJxePNWqyXioIaca44yXNbNPHBkv+QF6PJcQU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=umoPxODr; 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="umoPxODr" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A8334C4CEF1; Tue, 2 Dec 2025 10:24:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1764671102; bh=McMIirB/3kBEvvRZPpsdhilMluP94hMyIECum5e3cYk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=umoPxODriqahmzZI9qf5mBPY/zBtYM6FWwnPvJV2ReErchPj46EIbpDmcHyrm3JIr ovtgUNtZUtD9NHeINj5DHle+vM+/Fii4buhlxY+CYq1NaFfCdGiaU0CPRyAVdcp7sL IKrXoIVcqu/6r0G1toaAImLqHZQwKETCqOnwZN4xXKiPo3O1+RtYg1S86lUocIf6xw JZ86m9iCOfJh3azW+OoPKAFiD6xU8Pm1OrWwJETzO2rR9ikHady6mDPSDL8vDv2Net j1VhEASqR4jAPLfCFgckbLlrPgpwZ8cmgTp16kI+d1RLrrAW6UvWURYn61V2o5uXqd fad3eZ+Ghk+uA== Message-ID: <2901df56-bf0c-4d08-b043-eca294b981f9@kernel.org> Date: Tue, 2 Dec 2025 11:24:56 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4] mm/memory hotplug/unplug: Optimize zone->contiguous update when changes pfn range To: "Li, Tianyou" , Oscar Salvador , Mike Rapoport , Wei Yang Cc: 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 References: <20251201132216.1636924-1-tianyou.li@intel.com> <7633c77b-44eb-41f0-9c3a-1e5034b594e3@kernel.org> <0d9da08d-4293-4dbd-bf59-999488d73763@intel.com> From: "David Hildenbrand (Red Hat)" Content-Language: en-US In-Reply-To: <0d9da08d-4293-4dbd-bf59-999488d73763@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit >>> +}; >> >> 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. > >>> + >>> +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. -- Cheers David