Re: [PATCH] mm/vmstat: Fix a W=1 clang compiler warning

Re: [PATCH] mm/vmstat: Fix a W=1 clang compiler warning

[Ivan Shapovalov]

> Spose so.  One always suspects that adding a typecast is a sign that we
> screwed things up somehow.  The relationship between enums lru_list and
> node_stat_item is foggy, and I'm unsure whether this is the place to
> make the transition it.  Perhaps lru_list_name() should take an
> `unsigned int' arg instead.

All of these *_name() functions do seem to expect arguments in range of
the corresponding enums, so perhaps keep those args typed as a form of
self-documenting code, and do this instead?

---
From f8960df01b7f4fdc8d09a366886563385aed7f18 Mon Sep 17 00:00:00 2001
From: Ivan Shapovalov <intelfx@intelfx.name>
Date: Wed, 22 Jan 2025 04:10:59 +0300
Subject: [PATCH] mm: fix more clang compiler warnings

In the similar vein as 30c2de0a267c ("mm/vmstat: fix a W=1 clang
compiler warning"), fix several more sites that generate warnings
with clang 19.1.7:

./include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
  504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  505 |                            item];
      |                            ~~~~
./include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
  511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  512 |                            NR_VM_NUMA_EVENT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~~
./include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
  524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  525 |                            NR_VM_NUMA_EVENT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.

Similarly for mm_inline.h:

./include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
   47 |         __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
      |                                    ~~~~~~~~~~~ ^ ~~~
./include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
   49 |                                 NR_ZONE_LRU_BASE + lru, nr_pages);
      |                                 ~~~~~~~~~~~~~~~~ ^ ~~~
2 warnings generated.

Additionally, change the lru_list_name() and mm_inline.h sites to use
the same `(int)ENUMERATOR + ...` idiom to match the three first fixes
above for consistency. Everywhere else is's semantically more correct
thing to do because we are not really treating any of these enums as
other enums but rather treating all of those as indices.

Link: https://lore.kernel.org/all/20241212213126.1269116-1-bvanassche@acm.org/
Fixes: 9d7ea9a297e6 ("mm/vmstat: add helpers to get vmstat item names for each enum type")
Fixes: 30c2de0a267c ("mm/vmstat: fix a W=1 clang compiler warning")
Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name>
---
 include/linux/mm_inline.h | 4 ++--
 include/linux/vmstat.h    | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index 1b6a917fffa4..5a2328728b21 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -44,9 +44,9 @@ static __always_inline void __update_lru_size(struct lruvec *lruvec,
 	lockdep_assert_held(&lruvec->lru_lock);
 	WARN_ON_ONCE(nr_pages != (int)nr_pages);
 
-	__mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
+	__mod_lruvec_state(lruvec, (int)NR_LRU_BASE + lru, nr_pages);
 	__mod_zone_page_state(&pgdat->node_zones[zid],
-				NR_ZONE_LRU_BASE + lru, nr_pages);
+				(int)NR_ZONE_LRU_BASE + lru, nr_pages);
 }
 
 static __always_inline void update_lru_size(struct lruvec *lruvec,
diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index 9f3a04345b86..545568a4ea16 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -501,27 +501,27 @@ static inline const char *zone_stat_name(enum zone_stat_item item)
 #ifdef CONFIG_NUMA
 static inline const char *numa_stat_name(enum numa_stat_item item)
 {
-	return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
+	return vmstat_text[(int)NR_VM_ZONE_STAT_ITEMS +
 			   item];
 }
 #endif /* CONFIG_NUMA */
 
 static inline const char *node_stat_name(enum node_stat_item item)
 {
-	return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
+	return vmstat_text[(int)NR_VM_ZONE_STAT_ITEMS +
 			   NR_VM_NUMA_EVENT_ITEMS +
 			   item];
 }
 
 static inline const char *lru_list_name(enum lru_list lru)
 {
-	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // skip "nr_"
+	return node_stat_name((int)NR_LRU_BASE + lru) + 3; // skip "nr_"
 }
 
 #if defined(CONFIG_VM_EVENT_COUNTERS) || defined(CONFIG_MEMCG)
 static inline const char *vm_event_name(enum vm_event_item item)
 {
-	return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
+	return vmstat_text[(int)NR_VM_ZONE_STAT_ITEMS +
 			   NR_VM_NUMA_EVENT_ITEMS +
 			   NR_VM_NODE_STAT_ITEMS +
 			   NR_VM_STAT_ITEMS +
-- 
2.48.1.5.g9188e14f140

Re: [PATCH] mm/vmstat: Fix a W=1 clang compiler warning

[Bart Van Assche]

On 1/21/25 5:57 PM, Ivan Shapovalov wrote:
>> Spose so.  One always suspects that adding a typecast is a sign that we
>> screwed things up somehow.  The relationship between enums lru_list and
>> node_stat_item is foggy, and I'm unsure whether this is the place to
>> make the transition it.  Perhaps lru_list_name() should take an
>> `unsigned int' arg instead.
> 
> All of these *_name() functions do seem to expect arguments in range of
> the corresponding enums, so perhaps keep those args typed as a form of
> self-documenting code, and do this instead?
> 
> [ ... ]

Please take a look at
https://lore.kernel.org/linux-mm/8640d744-d182-474b-9059-204bcea47d1a@acm.org/. 
I think the patch I posted on December 13
requires fewer changes and no type casts.

Thanks,

Bart.

Re: [PATCH] mm/vmstat: Fix a W=1 clang compiler warning

[Bart Van Assche]

On 1/21/25 5:57 PM, Ivan Shapovalov wrote:
>> Spose so.  One always suspects that adding a typecast is a sign that we
>> screwed things up somehow.  The relationship between enums lru_list and
>> node_stat_item is foggy, and I'm unsure whether this is the place to
>> make the transition it.  Perhaps lru_list_name() should take an
>> `unsigned int' arg instead.
> 
> All of these *_name() functions do seem to expect arguments in range of
> the corresponding enums, so perhaps keep those args typed as a form of
> self-documenting code, and do this instead?

If nobody objects I will submit this patch for review after the merge
window has closed:

Subject: [PATCH] mm/vmstat: Fix W=1 clang compiler warnings

Commit 30c2de0a267c ("mm/vmstat: fix a W=1 clang compiler warning")
suppresses some but not all compiler warnings that are reported by clang
when building with W=1 about NR_LRU_BASE and NR_ZONE_LRU_BASE. Hence
revert commit 30c2de0a267c and instead make NR_LRU_BASE and
NR_ZONE_LRU_BASE integer constants instead of enumeration constants.

Cc: Ivan Shapovalov <intelfx@intelfx.name>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  include/linux/mmzone.h | 22 +++++++++++++++++++---
  include/linux/vmstat.h |  9 +++++++--
  2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 9540b41894da..92ed919ea99d 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -135,10 +135,19 @@ enum numa_stat_item {
  #define NR_VM_NUMA_EVENT_ITEMS 0
  #endif

+/*
+ * NR_ZONE_LRU_BASE and NR_VM_ZONE_STAT_ITEMS are often added to 
enumeration
+ * constants of another type than enum_zone_stat_item. Define these 
constants
+ * as an integer instead of enum node_stat_item to prevent that the 
compiler
+ * warns about enumeration type mismatches when these constants are used.
+ */
+#define NR_ZONE_LRU_BASE	(1 * __NR_ZONE_LRU_BASE)
+#define NR_VM_ZONE_STAT_ITEMS	(1 * __NR_VM_ZONE_STAT_ITEMS)
+
  enum zone_stat_item {
  	/* First 128 byte cacheline (assuming 64 bit words) */
  	NR_FREE_PAGES,
-	NR_ZONE_LRU_BASE, /* Used only for compaction and reclaim retry */
+	__NR_ZONE_LRU_BASE, /* Used only for compaction and reclaim retry */
  	NR_ZONE_INACTIVE_ANON = NR_ZONE_LRU_BASE,
  	NR_ZONE_ACTIVE_ANON,
  	NR_ZONE_INACTIVE_FILE,
@@ -155,10 +164,17 @@ enum zone_stat_item {
  #ifdef CONFIG_UNACCEPTED_MEMORY
  	NR_UNACCEPTED,
  #endif
-	NR_VM_ZONE_STAT_ITEMS };
+	__NR_VM_ZONE_STAT_ITEMS };
+
+/*
+ * enum lru_list constants are often added to NR_LRU_BASE. Define 
NR_LRU_BASE
+ * as an integer instead of enum node_stat_item to prevent that the 
compiler
+ * warns about enumeration type mismatches.
+ */
+#define NR_LRU_BASE (1 * __NR_LRU_BASE)

  enum node_stat_item {
-	NR_LRU_BASE,
+	__NR_LRU_BASE,
  	NR_INACTIVE_ANON = NR_LRU_BASE, /* must match order of LRU_[IN]ACTIVE */
  	NR_ACTIVE_ANON,		/*  "     "     "   "       "         */
  	NR_INACTIVE_FILE,	/*  "     "     "   "       "         */
diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index 9f3a04345b86..eb115b9a50f4 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -135,8 +135,13 @@ static inline void vm_events_fold_cpu(int cpu)
  #define count_vm_vma_lock_event(x) do {} while (0)
  #endif

+/*
+ * item##_NORMAL has type enum vm_event_item while ZONE_NORMAL and zid have
+ * type enum zone_type. Suppress compiler warnings about mixing different
+ * enumeration types by converting item##_NORMAL into an int with '1 *'.
+ */
  #define __count_zid_vm_events(item, zid, delta) \
-	__count_vm_events(item##_NORMAL - ZONE_NORMAL + zid, delta)
+	__count_vm_events(1 * item##_NORMAL - ZONE_NORMAL + zid, delta)

  /*
   * Zone and node-based page accounting with per cpu differentials.
@@ -515,7 +520,7 @@ static inline const char *node_stat_name(enum 
node_stat_item item)

  static inline const char *lru_list_name(enum lru_list lru)
  {
-	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // 
skip "nr_"
+	return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
  }

  #if defined(CONFIG_VM_EVENT_COUNTERS) || defined(CONFIG_MEMCG)

Re: [PATCH] mm/vmstat: Fix a W=1 clang compiler warning

[Vlastimil Babka]

On 1/28/25 22:36, Bart Van Assche wrote:
> On 1/21/25 5:57 PM, Ivan Shapovalov wrote:
>>> Spose so.  One always suspects that adding a typecast is a sign that we
>>> screwed things up somehow.  The relationship between enums lru_list and
>>> node_stat_item is foggy, and I'm unsure whether this is the place to
>>> make the transition it.  Perhaps lru_list_name() should take an
>>> `unsigned int' arg instead.
>> 
>> All of these *_name() functions do seem to expect arguments in range of
>> the corresponding enums, so perhaps keep those args typed as a form of
>> self-documenting code, and do this instead?
> 
> If nobody objects I will submit this patch for review after the merge
> window has closed:
> 
> Subject: [PATCH] mm/vmstat: Fix W=1 clang compiler warnings
> 
> Commit 30c2de0a267c ("mm/vmstat: fix a W=1 clang compiler warning")
> suppresses some but not all compiler warnings that are reported by clang
> when building with W=1 about NR_LRU_BASE and NR_ZONE_LRU_BASE. Hence
> revert commit 30c2de0a267c and instead make NR_LRU_BASE and
> NR_ZONE_LRU_BASE integer constants instead of enumeration constants.
> 
> Cc: Ivan Shapovalov <intelfx@intelfx.name>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   include/linux/mmzone.h | 22 +++++++++++++++++++---
>   include/linux/vmstat.h |  9 +++++++--
>   2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 9540b41894da..92ed919ea99d 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -135,10 +135,19 @@ enum numa_stat_item {
>   #define NR_VM_NUMA_EVENT_ITEMS 0
>   #endif
> 
> +/*
> + * NR_ZONE_LRU_BASE and NR_VM_ZONE_STAT_ITEMS are often added to 
> enumeration
> + * constants of another type than enum_zone_stat_item. Define these 
> constants
> + * as an integer instead of enum node_stat_item to prevent that the 
> compiler
> + * warns about enumeration type mismatches when these constants are used.
> + */
> +#define NR_ZONE_LRU_BASE	(1 * __NR_ZONE_LRU_BASE)
> +#define NR_VM_ZONE_STAT_ITEMS	(1 * __NR_VM_ZONE_STAT_ITEMS)

Seems an acceptable approach, dunno if this multiply by one is any better
than casting to int?

> +
>   enum zone_stat_item {
>   	/* First 128 byte cacheline (assuming 64 bit words) */
>   	NR_FREE_PAGES,
> -	NR_ZONE_LRU_BASE, /* Used only for compaction and reclaim retry */
> +	__NR_ZONE_LRU_BASE, /* Used only for compaction and reclaim retry */
>   	NR_ZONE_INACTIVE_ANON = NR_ZONE_LRU_BASE,

Should we rather use __NR_ZONE_LRU_BASE here?

>   	NR_ZONE_ACTIVE_ANON,
>   	NR_ZONE_INACTIVE_FILE,
> @@ -155,10 +164,17 @@ enum zone_stat_item {
>   #ifdef CONFIG_UNACCEPTED_MEMORY
>   	NR_UNACCEPTED,
>   #endif
> -	NR_VM_ZONE_STAT_ITEMS };
> +	__NR_VM_ZONE_STAT_ITEMS };
> +
> +/*
> + * enum lru_list constants are often added to NR_LRU_BASE. Define 
> NR_LRU_BASE
> + * as an integer instead of enum node_stat_item to prevent that the 
> compiler
> + * warns about enumeration type mismatches.
> + */
> +#define NR_LRU_BASE (1 * __NR_LRU_BASE)
> 
>   enum node_stat_item {
> -	NR_LRU_BASE,
> +	__NR_LRU_BASE,
>   	NR_INACTIVE_ANON = NR_LRU_BASE, /* must match order of LRU_[IN]ACTIVE */

ditto

>   	NR_ACTIVE_ANON,		/*  "     "     "   "       "         */
>   	NR_INACTIVE_FILE,	/*  "     "     "   "       "         */
> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> index 9f3a04345b86..eb115b9a50f4 100644
> --- a/include/linux/vmstat.h
> +++ b/include/linux/vmstat.h
> @@ -135,8 +135,13 @@ static inline void vm_events_fold_cpu(int cpu)
>   #define count_vm_vma_lock_event(x) do {} while (0)
>   #endif
> 
> +/*
> + * item##_NORMAL has type enum vm_event_item while ZONE_NORMAL and zid have
> + * type enum zone_type. Suppress compiler warnings about mixing different
> + * enumeration types by converting item##_NORMAL into an int with '1 *'.
> + */
>   #define __count_zid_vm_events(item, zid, delta) \
> -	__count_vm_events(item##_NORMAL - ZONE_NORMAL + zid, delta)
> +	__count_vm_events(1 * item##_NORMAL - ZONE_NORMAL + zid, delta)
> 
>   /*
>    * Zone and node-based page accounting with per cpu differentials.
> @@ -515,7 +520,7 @@ static inline const char *node_stat_name(enum 
> node_stat_item item)
> 
>   static inline const char *lru_list_name(enum lru_list lru)
>   {
> -	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // 
> skip "nr_"
> +	return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
>   }
> 
>   #if defined(CONFIG_VM_EVENT_COUNTERS) || defined(CONFIG_MEMCG)
> 

Re: [PATCH] mm/vmstat: Fix a W=1 clang compiler warning

[Bart Van Assche]

On 1/29/25 2:22 AM, Vlastimil Babka wrote:
> On 1/28/25 22:36, Bart Van Assche wrote:
>> +#define NR_ZONE_LRU_BASE	(1 * __NR_ZONE_LRU_BASE)
>> +#define NR_VM_ZONE_STAT_ITEMS	(1 * __NR_VM_ZONE_STAT_ITEMS)
> 
> Seems an acceptable approach, dunno if this multiply by one is any better
> than casting to int?

For enumeration types that are wider than an int, casting to an int
causes the highest bits to be lost. Multiplying by one doesn't have this
disadvantage. Please note that I don't have a strong opinion about this
and that casting to int would also work here.

>> +
>>    enum zone_stat_item {
>>    	/* First 128 byte cacheline (assuming 64 bit words) */
>>    	NR_FREE_PAGES,
>> -	NR_ZONE_LRU_BASE, /* Used only for compaction and reclaim retry */
>> +	__NR_ZONE_LRU_BASE, /* Used only for compaction and reclaim retry */
>>    	NR_ZONE_INACTIVE_ANON = NR_ZONE_LRU_BASE,
> 
> Should we rather use __NR_ZONE_LRU_BASE here?

Right, I will fix this and also a similar issue below.

Thanks,

Bart.

Re: [PATCH] mm/vmstat: Fix a W=1 clang compiler warning

[David Laight]

On Wed, 29 Jan 2025 11:22:00 +0100
Vlastimil Babka <vbabka@suse.cz> wrote:

> On 1/28/25 22:36, Bart Van Assche wrote:
> > On 1/21/25 5:57 PM, Ivan Shapovalov wrote:  
> >>> Spose so.  One always suspects that adding a typecast is a sign that we
> >>> screwed things up somehow.  The relationship between enums lru_list and
> >>> node_stat_item is foggy, and I'm unsure whether this is the place to
> >>> make the transition it.  Perhaps lru_list_name() should take an
> >>> `unsigned int' arg instead.  
> >> 
> >> All of these *_name() functions do seem to expect arguments in range of
> >> the corresponding enums, so perhaps keep those args typed as a form of
> >> self-documenting code, and do this instead?  
> > 
> > If nobody objects I will submit this patch for review after the merge
> > window has closed:
> > 
> > Subject: [PATCH] mm/vmstat: Fix W=1 clang compiler warnings
> > 
> > Commit 30c2de0a267c ("mm/vmstat: fix a W=1 clang compiler warning")
> > suppresses some but not all compiler warnings that are reported by clang
> > when building with W=1 about NR_LRU_BASE and NR_ZONE_LRU_BASE. Hence
> > revert commit 30c2de0a267c and instead make NR_LRU_BASE and
> > NR_ZONE_LRU_BASE integer constants instead of enumeration constants.
> > 
> > Cc: Ivan Shapovalov <intelfx@intelfx.name>
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >   include/linux/mmzone.h | 22 +++++++++++++++++++---
> >   include/linux/vmstat.h |  9 +++++++--
> >   2 files changed, 26 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 9540b41894da..92ed919ea99d 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -135,10 +135,19 @@ enum numa_stat_item {
> >   #define NR_VM_NUMA_EVENT_ITEMS 0
> >   #endif
> > 
> > +/*
> > + * NR_ZONE_LRU_BASE and NR_VM_ZONE_STAT_ITEMS are often added to 
> > enumeration
> > + * constants of another type than enum_zone_stat_item. Define these 
> > constants
> > + * as an integer instead of enum node_stat_item to prevent that the 
> > compiler
> > + * warns about enumeration type mismatches when these constants are used.
> > + */
> > +#define NR_ZONE_LRU_BASE	(1 * __NR_ZONE_LRU_BASE)
> > +#define NR_VM_ZONE_STAT_ITEMS	(1 * __NR_VM_ZONE_STAT_ITEMS)  
> 
> Seems an acceptable approach, dunno if this multiply by one is any better
> than casting to int?

I'd probably use (enum_value + 0) rather than a multiply.
And, if you are going to use multiply, I think it should be (value * 1)
for the same reason that 'if (1 == x)' horrible.

	David