mm: random cleanups

[PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper

Posted by Luis Chamberlain 1 year, 11 months ago

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
+
 static inline struct zone *zonelist_zone(struct zoneref *zoneref)
 {
 	return zoneref->zone;
diff --git a/mm/compaction.c b/mm/compaction.c
index b961db601df4..023a09d0786d 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2151,16 +2151,11 @@ static unsigned int fragmentation_score_zone_weighted(struct zone *zone)
 static unsigned int fragmentation_score_node(pg_data_t *pgdat)
 {
 	unsigned int score = 0;
+	struct zone *zone;
 	int zoneid;
 
-	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
-		struct zone *zone;
-
-		zone = &pgdat->node_zones[zoneid];
-		if (!populated_zone(zone))
-			continue;
+	for_each_populated_zone_pgdat(zone, pgdat)
 		score += fragmentation_score_zone_weighted(zone);
-	}
 
 	return score;
 }
-- 
2.43.0

Re: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper

Posted by kernel test robot 1 year, 11 months ago

Hi Luis,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Luis-Chamberlain/mm-show_mem-simplify-ifdef-on-si_meminfo_node/20240314-085917
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240314005436.2962962-3-mcgrof%40kernel.org
patch subject: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper
config: openrisc-defconfig (https://download.01.org/0day-ci/archive/20240314/202403141931.2XJWI4fa-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240314/202403141931.2XJWI4fa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403141931.2XJWI4fa-lkp@intel.com/

All warnings (new ones prefixed by >>):

   mm/compaction.c: In function 'fragmentation_score_node':
>> mm/compaction.c:2252:13: warning: unused variable 'zoneid' [-Wunused-variable]
    2252 |         int zoneid;
         |             ^~~~~~


vim +/zoneid +2252 mm/compaction.c

  2240	
  2241	/*
  2242	 * The per-node proactive (background) compaction process is started by its
  2243	 * corresponding kcompactd thread when the node's fragmentation score
  2244	 * exceeds the high threshold. The compaction process remains active till
  2245	 * the node's score falls below the low threshold, or one of the back-off
  2246	 * conditions is met.
  2247	 */
  2248	static unsigned int fragmentation_score_node(pg_data_t *pgdat)
  2249	{
  2250		unsigned int score = 0;
  2251		struct zone *zone;
> 2252		int zoneid;
  2253	
  2254		for_each_populated_zone_pgdat(zone, pgdat)
  2255			score += fragmentation_score_zone_weighted(zone);
  2256	
  2257		return score;
  2258	}
  2259	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper

Posted by Baolin Wang 1 year, 11 months ago


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.

>   static inline struct zone *zonelist_zone(struct zoneref *zoneref)
>   {
>   	return zoneref->zone;
> diff --git a/mm/compaction.c b/mm/compaction.c
> index b961db601df4..023a09d0786d 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2151,16 +2151,11 @@ static unsigned int fragmentation_score_zone_weighted(struct zone *zone)
>   static unsigned int fragmentation_score_node(pg_data_t *pgdat)
>   {
>   	unsigned int score = 0;
> +	struct zone *zone;
>   	int zoneid;
>   
> -	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
> -		struct zone *zone;
> -
> -		zone = &pgdat->node_zones[zoneid];
> -		if (!populated_zone(zone))
> -			continue;
> +	for_each_populated_zone_pgdat(zone, pgdat)
>   		score += fragmentation_score_zone_weighted(zone);
> -	}
>   
>   	return score;
>   }

Re: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper

Posted by Luis Chamberlain 1 year, 11 months ago

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:

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 34b729fc751b..bd11d33ea14d 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1568,6 +1568,7 @@ static inline struct pglist_data *NODE_DATA(int nid)
 extern struct pglist_data *first_online_pgdat(void);
 extern struct pglist_data *next_online_pgdat(struct pglist_data *pgdat);
 extern struct zone *next_zone(struct zone *zone);
+extern struct zone *next_zone_pgdat(struct zone *zone, struct pglist_data *pgdat);
 
 /**
  * for_each_online_pgdat - helper macro to iterate over all online nodes
@@ -1600,7 +1601,7 @@ extern struct zone *next_zone(struct zone *zone);
 #define for_each_populated_zone_pgdat(zone, pgdat)	\
 	for (zone = pgdat->node_zones;			\
 	     zone;					\
-	     zone = next_zone(zone))			\
+	     zone = next_zone_pgdat(zone, pgdat))	\
 		if (!populated_zone(zone))		\
 			; /* do nothing */		\
 		else
diff --git a/mm/compaction.c b/mm/compaction.c
index 015126803017..96434f6fc1ad 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2152,7 +2152,6 @@ static unsigned int fragmentation_score_node(pg_data_t *pgdat)
 {
 	unsigned int score = 0;
 	struct zone *zone;
-	int zoneid;
 
 	for_each_populated_zone_pgdat(zone, pgdat)
 		score += fragmentation_score_zone_weighted(zone);
diff --git a/mm/mmzone.c b/mm/mmzone.c
index c01896eca736..043a6dc16c05 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -43,6 +43,18 @@ struct zone *next_zone(struct zone *zone)
 	return zone;
 }
 
+/*
+ * next_zone_pgdat - helper magic for for_each_zone() per node
+ */
+struct zone *next_zone_pgdat(struct zone *zone, struct pglist_data *pgdat)
+{
+	if (!zone || !pgdat)
+		return NULL;
+	if (zone < pgdat->node_zones + MAX_NR_ZONES - 1)
+		return ++zone;
+	return NULL;
+}
+
 static inline int zref_in_nodemask(struct zoneref *zref, nodemask_t *nodes)
 {
 #ifdef CONFIG_NUMA

Re: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper

Posted by Vlastimil Babka 1 year, 11 months ago

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?

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.

> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 34b729fc751b..bd11d33ea14d 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1568,6 +1568,7 @@ static inline struct pglist_data *NODE_DATA(int nid)
>  extern struct pglist_data *first_online_pgdat(void);
>  extern struct pglist_data *next_online_pgdat(struct pglist_data *pgdat);
>  extern struct zone *next_zone(struct zone *zone);
> +extern struct zone *next_zone_pgdat(struct zone *zone, struct pglist_data *pgdat);
>  
>  /**
>   * for_each_online_pgdat - helper macro to iterate over all online nodes
> @@ -1600,7 +1601,7 @@ extern struct zone *next_zone(struct zone *zone);
>  #define for_each_populated_zone_pgdat(zone, pgdat)	\
>  	for (zone = pgdat->node_zones;			\
>  	     zone;					\
> -	     zone = next_zone(zone))			\
> +	     zone = next_zone_pgdat(zone, pgdat))	\
>  		if (!populated_zone(zone))		\
>  			; /* do nothing */		\
>  		else
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 015126803017..96434f6fc1ad 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2152,7 +2152,6 @@ static unsigned int fragmentation_score_node(pg_data_t *pgdat)
>  {
>  	unsigned int score = 0;
>  	struct zone *zone;
> -	int zoneid;
>  
>  	for_each_populated_zone_pgdat(zone, pgdat)
>  		score += fragmentation_score_zone_weighted(zone);
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index c01896eca736..043a6dc16c05 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -43,6 +43,18 @@ struct zone *next_zone(struct zone *zone)
>  	return zone;
>  }
>  
> +/*
> + * next_zone_pgdat - helper magic for for_each_zone() per node
> + */
> +struct zone *next_zone_pgdat(struct zone *zone, struct pglist_data *pgdat)
> +{
> +	if (!zone || !pgdat)
> +		return NULL;
> +	if (zone < pgdat->node_zones + MAX_NR_ZONES - 1)
> +		return ++zone;
> +	return NULL;
> +}
> +
>  static inline int zref_in_nodemask(struct zoneref *zref, nodemask_t *nodes)
>  {
>  #ifdef CONFIG_NUMA

Re: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper

Posted by Luis Chamberlain 1 year, 11 months ago

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

Re: [PATCH 2/3] mm/compaction: add and use for_each_populated_zone_pgdat() helper

Posted by Baolin Wang 1 year, 11 months ago


On 2024/3/15 13: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:
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 34b729fc751b..bd11d33ea14d 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1568,6 +1568,7 @@ static inline struct pglist_data *NODE_DATA(int nid)
>   extern struct pglist_data *first_online_pgdat(void);
>   extern struct pglist_data *next_online_pgdat(struct pglist_data *pgdat);
>   extern struct zone *next_zone(struct zone *zone);
> +extern struct zone *next_zone_pgdat(struct zone *zone, struct pglist_data *pgdat);
>   
>   /**
>    * for_each_online_pgdat - helper macro to iterate over all online nodes
> @@ -1600,7 +1601,7 @@ extern struct zone *next_zone(struct zone *zone);
>   #define for_each_populated_zone_pgdat(zone, pgdat)	\
>   	for (zone = pgdat->node_zones;			\
>   	     zone;					\
> -	     zone = next_zone(zone))			\
> +	     zone = next_zone_pgdat(zone, pgdat))	\
>   		if (!populated_zone(zone))		\
>   			; /* do nothing */		\
>   		else
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 015126803017..96434f6fc1ad 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2152,7 +2152,6 @@ static unsigned int fragmentation_score_node(pg_data_t *pgdat)
>   {
>   	unsigned int score = 0;
>   	struct zone *zone;
> -	int zoneid;
>   
>   	for_each_populated_zone_pgdat(zone, pgdat)
>   		score += fragmentation_score_zone_weighted(zone);
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index c01896eca736..043a6dc16c05 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -43,6 +43,18 @@ struct zone *next_zone(struct zone *zone)
>   	return zone;
>   }
>   
> +/*
> + * next_zone_pgdat - helper magic for for_each_zone() per node
> + */
> +struct zone *next_zone_pgdat(struct zone *zone, struct pglist_data *pgdat)
> +{
> +	if (!zone || !pgdat)
> +		return NULL;

Seems unnecessary, you already accessed them before calling 
next_zone_pgdat(). Otherwise, looks good to me.

> +	if (zone < pgdat->node_zones + MAX_NR_ZONES - 1)
> +		return ++zone;
> +	return NULL;
> +}
> +
>   static inline int zref_in_nodemask(struct zoneref *zref, nodemask_t *nodes)
>   {
>   #ifdef CONFIG_NUMA