Received-SPF: pass (google.com: domain of linux-kernel+bounces-104745-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 15 Mar 2024 10:48:32 -0700
From: Luis Chamberlain <mcgrof@kernel.org>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>, akpm@linux-foundation.org,
	jhubbard@nvidia.com, mgorman@suse.de, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, dave@stgolabs.net,
	p.raghav@samsung.com, da.gomez@samsung.com
Subject: Re: [PATCH 2/3] mm/compaction: add and use
 for_each_populated_zone_pgdat() helper
Message-ID: <ZfSJ8GPjVY54G1V4@bombadil.infradead.org>
References: <20240314005436.2962962-1-mcgrof@kernel.org>
 <20240314005436.2962962-3-mcgrof@kernel.org>
 <ea5d5bb6-32a5-4161-95cd-3dc916576bf7@linux.alibaba.com>
 <ZfPe22p9U8PiRB0W@bombadil.infradead.org>
 <532c9e93-e8cc-4f6f-bf08-b5625022dd4f@suse.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <532c9e93-e8cc-4f6f-bf08-b5625022dd4f@suse.cz>
Sender: Luis Chamberlain <mcgrof@infradead.org>

On Fri, Mar 15, 2024 at 12:09:29PM +0100, Vlastimil Babka wrote:
> On 3/15/24 06:38, Luis Chamberlain wrote:
> > On Thu, Mar 14, 2024 at 03:19:45PM +0800, Baolin Wang wrote:
> >> 
> >> 
> >> On 2024/3/14 08:54, Luis Chamberlain wrote:
> >> > We can just wrap most of the work done on fragmentation_score_node()
> >> > into a pgdat helper for populated zones. Add the helper and use it.
> >> > 
> >> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> >> > ---
> >> >   include/linux/mmzone.h | 8 ++++++++
> >> >   mm/compaction.c        | 9 ++-------
> >> >   2 files changed, 10 insertions(+), 7 deletions(-)
> >> > 
> >> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> >> > index a497f189d988..1fd74c7100ec 100644
> >> > --- a/include/linux/mmzone.h
> >> > +++ b/include/linux/mmzone.h
> >> > @@ -1597,6 +1597,14 @@ extern struct zone *next_zone(struct zone *zone);
> >> >   			; /* do nothing */		\
> >> >   		else
> >> > +#define for_each_populated_zone_pgdat(zone, pgdat)	\
> >> > +	for (zone = pgdat->node_zones;			\
> >> > +	     zone;					\
> >> > +	     zone = next_zone(zone))			\
> >> > +		if (!populated_zone(zone))		\
> >> > +			; /* do nothing */		\
> >> > +		else
> >> 
> >> I think this will break the original logics, since the next_zone() will
> >> iterate over all memory zones, instead of only the memory zones of the
> >> specified node.
> > 
> > Definitely, thanks, so we'd need something like this in addition:
> 
> IMHO that's unnecessarily complex, why not just do the iteration all inline
> without this next_zone_pgdat() helper?

Sure.

> Also maybe you could find more users if you created just a
> for_each_zone_pgdat() and left the populated_zone() in the user? Otherwise
> it's quite a specific helper with just one user.

Indeed, this was the only immediately clear user for_each_populated_zone_pgdat()
which stood out, but I'll look more closely. I added this helper because in an
RFC patch I had another use case:

https://lkml.kernel.org/r/20240314005710.2964798-1-mcgrof@kernel.org

Granted this is just an RFC. So happy to drop this if we don't have other users

  Luis