[PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[Yajun Deng]

Instead of define an index and determining if the zone has memory,
introduce for_each_populated_zone_pgdat() helper that can be used
to iterate over each populated zone in pgdat, and convert the most
obvious users to it.

This patch has no functional change.

Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 drivers/base/memory.c  |  7 ++-----
 include/linux/mmzone.h |  8 ++++++++
 mm/compaction.c        | 36 +++++++-----------------------------
 mm/page-writeback.c    |  8 ++------
 4 files changed, 19 insertions(+), 40 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index b456ac213610..ad898b1c85c7 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -656,7 +656,6 @@ static struct zone *early_node_zone_for_memory_block(struct memory_block *mem,
 	const unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
 	struct zone *zone, *matching_zone = NULL;
 	pg_data_t *pgdat = NODE_DATA(nid);
-	int i;
 
 	/*
 	 * This logic only works for early memory, when the applicable zones
@@ -666,10 +665,8 @@ static struct zone *early_node_zone_for_memory_block(struct memory_block *mem,
 	 * zones that intersect with the memory block are actually applicable.
 	 * No need to look at the memmap.
 	 */
-	for (i = 0; i < MAX_NR_ZONES; i++) {
-		zone = pgdat->node_zones + i;
-		if (!populated_zone(zone))
-			continue;
+	for_each_populated_zone_pgdat(zone, pgdat, MAX_NR_ZONES) {
+
 		if (!zone_intersects(zone, start_pfn, nr_pages))
 			continue;
 		if (!matching_zone) {
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index a4889c9d4055..48e9f01c0b5d 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
 			; /* do nothing */		\
 		else
 
+#define for_each_populated_zone_pgdat(zone, pgdat, max) \
+	for (zone = pgdat->node_zones;                  \
+	     zone < pgdat->node_zones + max;            \
+	     zone++)                                    \
+		if (!populated_zone(zone))		\
+			; /* do nothing */		\
+		else
+
 static inline struct zone *zonelist_zone(struct zoneref *zoneref)
 {
 	return zoneref->zone;
diff --git a/mm/compaction.c b/mm/compaction.c
index c8bcdea15f5f..863f10c7e510 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -375,12 +375,9 @@ static void __reset_isolation_suitable(struct zone *zone)
 
 void reset_isolation_suitable(pg_data_t *pgdat)
 {
-	int zoneid;
+	struct zone *zone;
 
-	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
-		struct zone *zone = &pgdat->node_zones[zoneid];
-		if (!populated_zone(zone))
-			continue;
+	for_each_populated_zone_pgdat(zone, pgdat, MAX_NR_ZONES) {
 
 		/* Only flush if a full compaction finished recently */
 		if (zone->compact_blockskip_flush)
@@ -2046,14 +2043,10 @@ static unsigned int fragmentation_score_zone_weighted(struct zone *zone)
 static unsigned int fragmentation_score_node(pg_data_t *pgdat)
 {
 	unsigned int score = 0;
-	int zoneid;
+	struct zone *zone;
 
-	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
-		struct zone *zone;
+	for_each_populated_zone_pgdat(zone, pgdat, MAX_NR_ZONES) {
 
-		zone = &pgdat->node_zones[zoneid];
-		if (!populated_zone(zone))
-			continue;
 		score += fragmentation_score_zone_weighted(zone);
 	}
 
@@ -2681,7 +2674,6 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
  */
 static void proactive_compact_node(pg_data_t *pgdat)
 {
-	int zoneid;
 	struct zone *zone;
 	struct compact_control cc = {
 		.order = -1,
@@ -2692,10 +2684,7 @@ static void proactive_compact_node(pg_data_t *pgdat)
 		.proactive_compaction = true,
 	};
 
-	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
-		zone = &pgdat->node_zones[zoneid];
-		if (!populated_zone(zone))
-			continue;
+	for_each_populated_zone_pgdat(zone, pgdat, MAX_NR_ZONES) {
 
 		cc.zone = zone;
 
@@ -2712,7 +2701,6 @@ static void proactive_compact_node(pg_data_t *pgdat)
 static void compact_node(int nid)
 {
 	pg_data_t *pgdat = NODE_DATA(nid);
-	int zoneid;
 	struct zone *zone;
 	struct compact_control cc = {
 		.order = -1,
@@ -2722,12 +2710,7 @@ static void compact_node(int nid)
 		.gfp_mask = GFP_KERNEL,
 	};
 
-
-	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
-
-		zone = &pgdat->node_zones[zoneid];
-		if (!populated_zone(zone))
-			continue;
+	for_each_populated_zone_pgdat(zone, pgdat, MAX_NR_ZONES) {
 
 		cc.zone = zone;
 
@@ -2823,15 +2806,10 @@ static inline bool kcompactd_work_requested(pg_data_t *pgdat)
 
 static bool kcompactd_node_suitable(pg_data_t *pgdat)
 {
-	int zoneid;
 	struct zone *zone;
 	enum zone_type highest_zoneidx = pgdat->kcompactd_highest_zoneidx;
 
-	for (zoneid = 0; zoneid <= highest_zoneidx; zoneid++) {
-		zone = &pgdat->node_zones[zoneid];
-
-		if (!populated_zone(zone))
-			continue;
+	for_each_populated_zone_pgdat(zone, pgdat, highest_zoneidx + 1) {
 
 		if (compaction_suitable(zone, pgdat->kcompactd_max_order, 0,
 					highest_zoneidx) == COMPACT_CONTINUE)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index db7943999007..9a7bcf8fdfd5 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -272,13 +272,9 @@ static void wb_min_max_ratio(struct bdi_writeback *wb,
 static unsigned long node_dirtyable_memory(struct pglist_data *pgdat)
 {
 	unsigned long nr_pages = 0;
-	int z;
+	struct zone *zone;
 
-	for (z = 0; z < MAX_NR_ZONES; z++) {
-		struct zone *zone = pgdat->node_zones + z;
-
-		if (!populated_zone(zone))
-			continue;
+	for_each_populated_zone_pgdat(zone, pgdat, MAX_NR_ZONES) {
 
 		nr_pages += zone_page_state(zone, NR_FREE_PAGES);
 	}
-- 
2.25.1

Re: [PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[Matthew Wilcox]

On Mon, Apr 24, 2023 at 11:07:56AM +0800, Yajun Deng wrote:
> Instead of define an index and determining if the zone has memory,
> introduce for_each_populated_zone_pgdat() helper that can be used
> to iterate over each populated zone in pgdat, and convert the most
> obvious users to it.

I don't think the complexity of the helper justifies the simplification
of the users.

> +++ b/include/linux/mmzone.h
> @@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
>  			; /* do nothing */		\
>  		else
>  
> +#define for_each_populated_zone_pgdat(zone, pgdat, max) \
> +	for (zone = pgdat->node_zones;                  \
> +	     zone < pgdat->node_zones + max;            \
> +	     zone++)                                    \
> +		if (!populated_zone(zone))		\
> +			; /* do nothing */		\
> +		else
> +

Re: [PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[Andrew Morton]

On Mon, 24 Apr 2023 04:50:37 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Apr 24, 2023 at 11:07:56AM +0800, Yajun Deng wrote:
> > Instead of define an index and determining if the zone has memory,
> > introduce for_each_populated_zone_pgdat() helper that can be used
> > to iterate over each populated zone in pgdat, and convert the most
> > obvious users to it.
> 
> I don't think the complexity of the helper justifies the simplification
> of the users.

Are you sure?

> > +++ b/include/linux/mmzone.h
> > @@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
> >  			; /* do nothing */		\
> >  		else
> >  
> > +#define for_each_populated_zone_pgdat(zone, pgdat, max) \
> > +	for (zone = pgdat->node_zones;                  \
> > +	     zone < pgdat->node_zones + max;            \
> > +	     zone++)                                    \
> > +		if (!populated_zone(zone))		\
> > +			; /* do nothing */		\
> > +		else
> > +

But each of the call sites is doing this, so at least the complexity is
now seen in only one place.

btw, do we need to do the test that way?  Why won't this work?

#define for_each_populated_zone_pgdat(zone, pgdat, max) \
	for (zone = pgdat->node_zones;                  \
	     zone < pgdat->node_zones + max;            \
	     zone++)                                    \
		if (populated_zone(zone))

I suspect it was done the original way in order to save a tabstop,
which is no longer needed.

Re: [PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[Matthew Wilcox]

On Mon, Apr 24, 2023 at 02:58:23PM -0700, Andrew Morton wrote:
> On Mon, 24 Apr 2023 04:50:37 +0100 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Mon, Apr 24, 2023 at 11:07:56AM +0800, Yajun Deng wrote:
> > > Instead of define an index and determining if the zone has memory,
> > > introduce for_each_populated_zone_pgdat() helper that can be used
> > > to iterate over each populated zone in pgdat, and convert the most
> > > obvious users to it.
> > 
> > I don't think the complexity of the helper justifies the simplification
> > of the users.
> 
> Are you sure?
> 
> > > +++ b/include/linux/mmzone.h
> > > @@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
> > >  			; /* do nothing */		\
> > >  		else
> > >  
> > > +#define for_each_populated_zone_pgdat(zone, pgdat, max) \
> > > +	for (zone = pgdat->node_zones;                  \
> > > +	     zone < pgdat->node_zones + max;            \
> > > +	     zone++)                                    \
> > > +		if (!populated_zone(zone))		\
> > > +			; /* do nothing */		\
> > > +		else
> > > +
> 
> But each of the call sites is doing this, so at least the complexity is
> now seen in only one place.

But they're not doing _that_.  They're doing something normal and
obvious like:

	for (zone = pgdat->node_zones; zone < pgdat->node_zones + max; zone++) {
		if (!populated_zone(zone)
			continue;
		...
	}

which clearly does what it's supposed to.  But with this patch, there's
macro expansion involved, and it's not a nice simple macro, it has a loop
_and_ an if-condition, and there's an else, and now I have to think hard
about whether flow control is going to do the right thing if the body
of the loop isn't simple.

> btw, do we need to do the test that way?  Why won't this work?
> 
> #define for_each_populated_zone_pgdat(zone, pgdat, max) \
> 	for (zone = pgdat->node_zones;                  \
> 	     zone < pgdat->node_zones + max;            \
> 	     zone++)                                    \
> 		if (populated_zone(zone))

I think it will work, except that this is now legal:

	for_each_populated_zone_pgdat(zone, pgdat, 3)
	else i++;

and really, I think that demonstrates why we don't want macros that are
that darn clever.

Re: [PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[Huang, Ying]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Mon, 24 Apr 2023 04:50:37 +0100 Matthew Wilcox <willy@infradead.org> wrote:
>
>> On Mon, Apr 24, 2023 at 11:07:56AM +0800, Yajun Deng wrote:
>> > Instead of define an index and determining if the zone has memory,
>> > introduce for_each_populated_zone_pgdat() helper that can be used
>> > to iterate over each populated zone in pgdat, and convert the most
>> > obvious users to it.
>> 
>> I don't think the complexity of the helper justifies the simplification
>> of the users.
>
> Are you sure?
>
>> > +++ b/include/linux/mmzone.h
>> > @@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
>> >  			; /* do nothing */		\
>> >  		else
>> >  
>> > +#define for_each_populated_zone_pgdat(zone, pgdat, max) \
>> > +	for (zone = pgdat->node_zones;                  \
>> > +	     zone < pgdat->node_zones + max;            \
>> > +	     zone++)                                    \
>> > +		if (!populated_zone(zone))		\
>> > +			; /* do nothing */		\
>> > +		else
>> > +
>
> But each of the call sites is doing this, so at least the complexity is
> now seen in only one place.
>
> btw, do we need to do the test that way?  Why won't this work?
>
> #define for_each_populated_zone_pgdat(zone, pgdat, max) \
> 	for (zone = pgdat->node_zones;                  \
> 	     zone < pgdat->node_zones + max;            \
> 	     zone++)                                    \
> 		if (populated_zone(zone))
>
> I suspect it was done the original way in order to save a tabstop,
> which is no longer needed.

This may cause unexpected effect when used with "if" statement.  For
example,

        if (something)
                for_each_populated_zone_pgdat(zone, pgdat, max)
                        total += zone->present_pages;
        else
                pr_info("something is false!\n");

Best Regards,
Huang, Ying

Re: [PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[Yajun Deng]

April 25, 2023 11:23 AM, "Matthew Wilcox" <willy@infradead.org> wrote:

> On Mon, Apr 24, 2023 at 02:58:23PM -0700, Andrew Morton wrote:
> 
>> On Mon, 24 Apr 2023 04:50:37 +0100 Matthew Wilcox <willy@infradead.org> wrote:
>> 
>> On Mon, Apr 24, 2023 at 11:07:56AM +0800, Yajun Deng wrote:
>>> Instead of define an index and determining if the zone has memory,
>>> introduce for_each_populated_zone_pgdat() helper that can be used
>>> to iterate over each populated zone in pgdat, and convert the most
>>> obvious users to it.
>> 
>> I don't think the complexity of the helper justifies the simplification
>> of the users.
>> 
>> Are you sure?
>> 
>>> +++ b/include/linux/mmzone.h
>>> @@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
>>> ; /* do nothing */ \
>>> else
>>> 
>>> +#define for_each_populated_zone_pgdat(zone, pgdat, max) \
>>> + for (zone = pgdat->node_zones; \
>>> + zone < pgdat->node_zones + max; \
>>> + zone++) \
>>> + if (!populated_zone(zone)) \
>>> + ; /* do nothing */ \
>>> + else
>>> +
>> 
>> But each of the call sites is doing this, so at least the complexity is
>> now seen in only one place.
> 
> But they're not doing _that_. They're doing something normal and
> obvious like:
> 
> for (zone = pgdat->node_zones; zone < pgdat->node_zones + max; zone++) {
> if (!populated_zone(zone)
> continue;
> ...
> }
>

They will be like:

for (zone = pgdat->node_zones; zone < pgdat->node_zones + max; zone++)
        if (!populated_zone(zone))
                ;
        else {

                ...
        }
     
 
> which clearly does what it's supposed to. But with this patch, there's
> macro expansion involved, and it's not a nice simple macro, it has a loop
> _and_ an if-condition, and there's an else, and now I have to think hard
> about whether flow control is going to do the right thing if the body
> of the loop isn't simple.
> 
>> btw, do we need to do the test that way? Why won't this work?
>> 
>> #define for_each_populated_zone_pgdat(zone, pgdat, max) \
>> for (zone = pgdat->node_zones; \
>> zone < pgdat->node_zones + max; \
>> zone++) \
>> if (populated_zone(zone))
> 
> I think it will work, except that this is now legal:
> 
> for_each_populated_zone_pgdat(zone, pgdat, 3)
> else i++;
> 
> and really, I think that demonstrates why we don't want macros that are
> that darn clever.

Re: [PATCH] mmzone: Introduce for_each_populated_zone_pgdat()

[Yajun Deng]

April 25, 2023 1:51 PM, "Huang, Ying" <ying.huang@intel.com> wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
>> On Mon, 24 Apr 2023 04:50:37 +0100 Matthew Wilcox <willy@infradead.org> wrote:
>> 
>>> On Mon, Apr 24, 2023 at 11:07:56AM +0800, Yajun Deng wrote:
>>>> Instead of define an index and determining if the zone has memory,
>>>> introduce for_each_populated_zone_pgdat() helper that can be used
>>>> to iterate over each populated zone in pgdat, and convert the most
>>>> obvious users to it.
>>> 
>>> I don't think the complexity of the helper justifies the simplification
>>> of the users.
>> 
>> Are you sure?
>> 
>>>> +++ b/include/linux/mmzone.h
>>>> @@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
>>>> ; /* do nothing */ \
>>>> else
>>>> 
>>>> +#define for_each_populated_zone_pgdat(zone, pgdat, max) \
>>>> + for (zone = pgdat->node_zones; \
>>>> + zone < pgdat->node_zones + max; \
>>>> + zone++) \
>>>> + if (!populated_zone(zone)) \
>>>> + ; /* do nothing */ \
>>>> + else
>>>> +
>> 
>> But each of the call sites is doing this, so at least the complexity is
>> now seen in only one place.
>> 
>> btw, do we need to do the test that way? Why won't this work?
>> 
>> #define for_each_populated_zone_pgdat(zone, pgdat, max) \
>> for (zone = pgdat->node_zones; \
>> zone < pgdat->node_zones + max; \
>> zone++) \
>> if (populated_zone(zone))
>> 
>> I suspect it was done the original way in order to save a tabstop,
>> which is no longer needed.
> 
> This may cause unexpected effect when used with "if" statement. For
> example,
> 
> if (something)
> for_each_populated_zone_pgdat(zone, pgdat, max)
> total += zone->present_pages;
> else
> pr_info("something is false!\n");
> 

Thanks Huang, Ying for the example.

Yes, this macros with multiple statements but doesn't have a do - while loop,
It needs if and else together.

> Best Regards,
> Huang, Ying