linux-kernel - Re: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZfSJ8GPjVY54G1V4@bombadil.infradead.org>
Date: Fri, 15 Mar 2024 10:48:32 -0700
From: Luis Chamberlain <mcgrof@...nel.org>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Baolin Wang <baolin.wang@...ux.alibaba.com>, akpm@...ux-foundation.org,
	jhubbard@...dia.com, mgorman@...e.de, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, dave@...olabs.net,
	p.raghav@...sung.com, da.gomez@...sung.com
Subject: Re: [PATCH 2/3] mm/compaction: add and use
 for_each_populated_zone_pgdat() helper

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@...nel.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