Here's a few random minor cleanups I spotted while reviewing a few mm/*.c files.
No functional changes.
Luis Chamberlain (3):
mm/show_mem: simplify ifdef on si_meminfo_node()
mm/compaction: add and use for_each_populated_zone_pgdat() helper
mm/vmstat: simplfy extfrag_show_print with fragmentation_index()
include/linux/mmzone.h | 8 ++++++++
mm/compaction.c | 9 ++-------
mm/show_mem.c | 5 +----
mm/vmstat.c | 11 +++++------
4 files changed, 16 insertions(+), 17 deletions(-)
--
2.43.0
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 <[email protected]>
---
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
fragmentation_index() already uses the stack for the struct contig_page_info,
so just use that and enhance the documentation for fragmentation_index().
Signed-off-by: Luis Chamberlain <[email protected]>
---
mm/vmstat.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/mm/vmstat.c b/mm/vmstat.c
index db79935e4a54..582f89b37ccf 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1129,7 +1129,10 @@ unsigned int extfrag_for_order(struct zone *zone, unsigned int order)
info.free_pages);
}
-/* Same as __fragmentation index but allocs contig_page_info on stack */
+/*
+ * Same as __fragmentation index but allocs contig_page_info on stack,
+ * useful when walking a zone as interrupts are disabled.
+ */
int fragmentation_index(struct zone *zone, unsigned int order)
{
struct contig_page_info info;
@@ -2227,15 +2230,11 @@ static void extfrag_show_print(struct seq_file *m,
unsigned int order;
int index;
- /* Alloc on stack as interrupts are disabled for zone walk */
- struct contig_page_info info;
-
seq_printf(m, "Node %d, zone %8s ",
pgdat->node_id,
zone->name);
for (order = 0; order < NR_PAGE_ORDERS; ++order) {
- fill_contig_page_info(zone, order, &info);
- index = __fragmentation_index(order, &info);
+ index = fragmentation_index(zone, order);
seq_printf(m, "%2d.%03d ", index / 1000, index % 1000);
}
--
2.43.0
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 <[email protected]>
> ---
> 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;
> }
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/[email protected]/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240314/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>> > ---
>> > 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
On 3/14/24 01:54, Luis Chamberlain wrote:
> fragmentation_index() already uses the stack for the struct contig_page_info,
> so just use that and enhance the documentation for fragmentation_index().
>
> Signed-off-by: Luis Chamberlain <[email protected]>
Reviewed-by: Vlastimil Babka <[email protected]>
> ---
> mm/vmstat.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index db79935e4a54..582f89b37ccf 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1129,7 +1129,10 @@ unsigned int extfrag_for_order(struct zone *zone, unsigned int order)
> info.free_pages);
> }
>
> -/* Same as __fragmentation index but allocs contig_page_info on stack */
> +/*
> + * Same as __fragmentation index but allocs contig_page_info on stack,
> + * useful when walking a zone as interrupts are disabled.
> + */
> int fragmentation_index(struct zone *zone, unsigned int order)
> {
> struct contig_page_info info;
> @@ -2227,15 +2230,11 @@ static void extfrag_show_print(struct seq_file *m,
> unsigned int order;
> int index;
>
> - /* Alloc on stack as interrupts are disabled for zone walk */
> - struct contig_page_info info;
> -
> seq_printf(m, "Node %d, zone %8s ",
> pgdat->node_id,
> zone->name);
> for (order = 0; order < NR_PAGE_ORDERS; ++order) {
> - fill_contig_page_info(zone, order, &info);
> - index = __fragmentation_index(order, &info);
> + index = fragmentation_index(zone, order);
> seq_printf(m, "%2d.%03d ", index / 1000, index % 1000);
> }
>
On Wed, 13 Mar 2024, Luis Chamberlain wrote:
>fragmentation_index() already uses the stack for the struct contig_page_info,
>so just use that and enhance the documentation for fragmentation_index().
>
Reviewed-by: Davidlohr Bueso <[email protected]>
>Signed-off-by: Luis Chamberlain <[email protected]>
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 <[email protected]>
> >> > ---
> >> > 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/[email protected]
Granted this is just an RFC. So happy to drop this if we don't have other users
Luis