[PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[Qingshuang Fu]

From: Qingshuang Fu <fuqingshuang@kylinos.cn>

Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
is int,but MAX_CR_TIERS is an unsigned type, directly using
the min function for comparison will result in an error:
from mm/vmscan.c:15:
mm/vmscan.c: In function ‘read_ctrl_pos’:
./include/linux/build_bug.h:78:41: error: static assertion failed:
"min(tier, 4U - 1) signedness error, fix types or
consider umin() before min_t()"
And MAX_CR_TIERS is a macro definition defined as 4U,
so min_t can be used to convert it to int type before
performing the minimum value operation.

Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7de11524a936..f991196fd8e5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3194,7 +3194,7 @@ static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
 	pos->gain = gain;
 	pos->refaulted = pos->total = 0;
 
-	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
+	for (i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) {
 		pos->refaulted += lrugen->avg_refaulted[type][i] +
 				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
 		pos->total += lrugen->avg_total[type][i] +
-- 
2.25.1

[PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[Qingshuang Fu]

From: Qingshuang Fu <fuqingshuang@kylinos.cn>

Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
is int,but MAX_CR_TIERS is an unsigned type, directly using
the min function for comparison will result in an error:
from mm/vmscan.c:15:
mm/vmscan.c: In function ‘read_ctrl_pos’:
./include/linux/build_bug.h:78:41: error: static assertion failed:
"min(tier, 4U - 1) signedness error, fix types or
consider umin() before min_t()"
And MAX_CR_TIERS is a macro definition defined as 4U,
so min_t can be used to convert it to int type before
performing the minimum value operation.

Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7de11524a936..f991196fd8e5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3194,7 +3194,7 @@ static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
 	pos->gain = gain;
 	pos->refaulted = pos->total = 0;
 
-	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
+	for (i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) {
 		pos->refaulted += lrugen->avg_refaulted[type][i] +
 				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
 		pos->total += lrugen->avg_total[type][i] +
-- 
2.25.1

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[David Hildenbrand]

On 08.08.25 09:21, Qingshuang Fu wrote:
> From: Qingshuang Fu <fuqingshuang@kylinos.cn>

Subject should probably be

"mm/vmscan: fix build bug in read_ctrl_pos"

> 
> Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
> is int,but MAX_CR_TIERS is an unsigned type, directly using
> the min function for comparison will result in an error:
> from mm/vmscan.c:15:
> mm/vmscan.c: In function ‘read_ctrl_pos’:
> ./include/linux/build_bug.h:78:41: error: static assertion failed:
> "min(tier, 4U - 1) signedness error, fix types or
> consider umin() before min_t()"
> And MAX_CR_TIERS is a macro definition defined as 4U,
> so min_t can be used to convert it to int type before
> performing the minimum value operation.
> 

Please use empty lines to make the description easier to read. Also, I 
think you can simplify this heavily.

We should add

Fixes: 37a260870f2c ("mm/mglru: rework type selection")

BUT

this commit is more than half a year old. How come no built bot 
complained about that?

IOW, what compiler are you using and why are only you able to trigger this>

> Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
> ---
>   mm/vmscan.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 7de11524a936..f991196fd8e5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3194,7 +3194,7 @@ static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
>   	pos->gain = gain;
>   	pos->refaulted = pos->total = 0;
>   
> -	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
> +	for (i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) {
>   		pos->refaulted += lrugen->avg_refaulted[type][i] +
>   				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
>   		pos->total += lrugen->avg_total[type][i] +


-- 
Cheers,

David / dhildenb

[PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[Qingshuang Fu]

From: Qingshuang Fu <fuqingshuang@kylinos.cn>

Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
is int,but MAX_CR_TIERS is an unsigned type, directly using
the min function for comparison will result in an error:

from mm/vmscan.c:15:
mm/vmscan.c: In function ‘read_ctrl_pos’:
./include/linux/build_bug.h:78:41: error: static assertion failed:
"min(tier, 4U - 1) signedness error, fix types or
consider umin() before min_t()"

Fixes: 37a260870f2c ("mm/mglru: rework type selection")
Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
Suggested-by: David Hildenbrand <david@redhat.com>

---
v1 -> v2:
Modified patch descriptio
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7de11524a936..f991196fd8e5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3194,7 +3194,7 @@ static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
 	pos->gain = gain;
 	pos->refaulted = pos->total = 0;
 
-	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
+	for (i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) {
 		pos->refaulted += lrugen->avg_refaulted[type][i] +
 				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
 		pos->total += lrugen->avg_total[type][i] +
-- 
2.25.1

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[David Hildenbrand]

On 08.08.25 11:26, fffsqian wrote:
> Hi David,
> 
> 
> Thanks a lot for your feedback!
> 
> 
> Regarding the question of why only I encountered this error: it’s 
> because my compilation environment has CONFIG_WERROR enabled (treating 
> warnings as errors). The original code triggers a static assertion 
> warning (due to the signed/unsigned mismatch in min(tier, 4U - 1)), 
> which would normally be a non-fatal warning. However, with 
> CONFIG_WERROR, this gets elevated to a compile error, making it visible 
> in my builds.

Does it?

$ make mm/vmscan.o
...
   CC      mm/vmscan.o
$ gcc --version
gcc (GCC) 15.1.1 20250521 (Red Hat 15.1.1-2)

What am I missing?

> 
> 
> The error message is as follows:
> 
> from mm/vmscan.c:15:
> 
> mm/vmscan.c: In function ‘read_ctrl_pos’:
> 
> ./include/linux/build_bug.h:78:41: error: static assertion failed: 
> "min(tier, 4U - 1) signedness error, fix types or consider umin() before 
> min_t()"
> 
> 
> I really appreciate your suggestions on the patch format issues—I’ve 
> addressed those as well, including adding fix information and 
> streamlining patch descriptions, etc.
> 
> 
> V2 of the patch will be sent shortly.

Please wait with v2 until the discussion is over. Also, flag your 
patches as v2 like [PATCH v2] etc.

-- 
Cheers,

David / dhildenb

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[David Hildenbrand]

On 08.08.25 11:27, Qingshuang Fu wrote:
> From: Qingshuang Fu <fuqingshuang@kylinos.cn>
> 

See my other reply, (e.g., versioning), how you have to provide more 
detail on how to *actually* reproduce this (I cannot) and you ignored my 
comment about the patch subject.

-- 
Cheers,

David / dhildenb

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[Andrew Morton]

On Fri, 8 Aug 2025 09:35:19 +0200 David Hildenbrand <david@redhat.com> wrote:

> > 
> > Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
> > is int,but MAX_CR_TIERS is an unsigned type, directly using
> > the min function for comparison will result in an error:
> > from mm/vmscan.c:15:
> > mm/vmscan.c: In function ‘read_ctrl_pos’:
> > ./include/linux/build_bug.h:78:41: error: static assertion failed:
> > "min(tier, 4U - 1) signedness error, fix types or
> > consider umin() before min_t()"
> > And MAX_CR_TIERS is a macro definition defined as 4U,
> > so min_t can be used to convert it to int type before
> > performing the minimum value operation.
> > 
> 
> Please use empty lines to make the description easier to read. Also, I 
> think you can simplify this heavily.
> 
> We should add
> 
> Fixes: 37a260870f2c ("mm/mglru: rework type selection")

I'm not liking read_ctrl_pos() much.

Local variable `i' has the rottenest possible name.  In this case it is
a "tier", so let's call it that.  And its type should be unsigned.

But an incoming arg has that name.  Probably inappropriately.  I'm
suspecting that something like base_tier or start_tier would be more
descriptive.  Hard to tell, because read_ctrl_pos() forgot to get
documented.  And this arg should have unsigned type also.

(cc mglru maintainers, who have yet to reveal themselves in MAINTAINERS)

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[Axel Rasmussen]

On Mon, Aug 11, 2025 at 9:40 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 8 Aug 2025 09:35:19 +0200 David Hildenbrand <david@redhat.com> wrote:
>
> > >
> > > Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
> > > is int,but MAX_CR_TIERS is an unsigned type, directly using
> > > the min function for comparison will result in an error:
> > > from mm/vmscan.c:15:
> > > mm/vmscan.c: In function ‘read_ctrl_pos’:
> > > ./include/linux/build_bug.h:78:41: error: static assertion failed:
> > > "min(tier, 4U - 1) signedness error, fix types or
> > > consider umin() before min_t()"
> > > And MAX_CR_TIERS is a macro definition defined as 4U,
> > > so min_t can be used to convert it to int type before
> > > performing the minimum value operation.
> > >
> >
> > Please use empty lines to make the description easier to read. Also, I
> > think you can simplify this heavily.
> >
> > We should add
> >
> > Fixes: 37a260870f2c ("mm/mglru: rework type selection")
>
> I'm not liking read_ctrl_pos() much.
>
> Local variable `i' has the rottenest possible name.  In this case it is
> a "tier", so let's call it that.  And its type should be unsigned.
>
> But an incoming arg has that name.  Probably inappropriately.  I'm
> suspecting that something like base_tier or start_tier would be more
> descriptive.  Hard to tell, because read_ctrl_pos() forgot to get
> documented.  And this arg should have unsigned type also.

I agree a more thorough rework to be consistent with types / make the
ctrl_pos helpers more readable would be better. As is, given David's
point about not being able to reproduce this without fiddling with
compiler flags, I wouldn't be in favor of taking this.

>
>
> (cc mglru maintainers, who have yet to reveal themselves in MAINTAINERS)

Sorry for that, I took a look at adding myself and Yuanchu, but it
wasn't clear how to represent MGLRU in MAINTAINERS, because there
isn't a mglru.c, it's bolted on to several existing files with
existing maintainers. Should us two just be appended to the list of
folks for vmscan.c, or?

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[Andrew Morton]

On Wed, 13 Aug 2025 13:03:42 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote:

> >
> >
> > (cc mglru maintainers, who have yet to reveal themselves in MAINTAINERS)
> 
> Sorry for that, I took a look at adding myself and Yuanchu, but it
> wasn't clear how to represent MGLRU in MAINTAINERS, because there
> isn't a mglru.c, it's bolted on to several existing files with
> existing maintainers. Should us two just be appended to the list of
> folks for vmscan.c, or?

I think a separate MGLRU section which mentions mm/vmscan.c is
appropriate.  kernel/fork.c gets a mention in three sections.

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[Lorenzo Stoakes]

On Wed, Aug 13, 2025 at 02:41:45PM -0700, Andrew Morton wrote:
> On Wed, 13 Aug 2025 13:03:42 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote:
>
> > >
> > >
> > > (cc mglru maintainers, who have yet to reveal themselves in MAINTAINERS)
> >
> > Sorry for that, I took a look at adding myself and Yuanchu, but it
> > wasn't clear how to represent MGLRU in MAINTAINERS, because there
> > isn't a mglru.c, it's bolted on to several existing files with
> > existing maintainers. Should us two just be appended to the list of
> > folks for vmscan.c, or?
>
> I think a separate MGLRU section which mentions mm/vmscan.c is
> appropriate.  kernel/fork.c gets a mention in three sections.
>

Indeed, it turns out you can have multiple entries with the same file(s),
which turned out to be very useful in assigning the covers-many-bases
kernel/fork.c.

Having added a bunch of the newer mm entries, may I humbly suggest
something like:

MEMORY MANAGEMENT - MGLRU (MULTI-GEN LRU)
M:	Andrew Morton <akpm@linux-foundation.org>
M:	Axel Rasmussen <axelrasmussen@google.com>
M:	Yuanchu Xie <yuanchu@google.com>
< figure out any appropriate reviewers here, if any >
L:	linux-mm@kvack.org
S:	Maintained
F:	mm/vmscan.c

Placed in the correct alphabetical location in MAINTAINERS?

Cheers, Lorenzo

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[David Laight]

On Fri, 8 Aug 2025 09:35:19 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.08.25 09:21, Qingshuang Fu wrote:
> > From: Qingshuang Fu <fuqingshuang@kylinos.cn>  
> 
> Subject should probably be
> 
> "mm/vmscan: fix build bug in read_ctrl_pos"
> 
> > 
> > Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
> > is int,but MAX_CR_TIERS is an unsigned type, directly using
> > the min function for comparison will result in an error:
> > from mm/vmscan.c:15:
> > mm/vmscan.c: In function ‘read_ctrl_pos’:
> > ./include/linux/build_bug.h:78:41: error: static assertion failed:
> > "min(tier, 4U - 1) signedness error, fix types or
> > consider umin() before min_t()"
> > And MAX_CR_TIERS is a macro definition defined as 4U,
> > so min_t can be used to convert it to int type before
> > performing the minimum value operation.
> >   
> 
> Please use empty lines to make the description easier to read. Also, I 
> think you can simplify this heavily.
> 
> We should add
> 
> Fixes: 37a260870f2c ("mm/mglru: rework type selection")
> 
> BUT
> 
> this commit is more than half a year old. How come no built bot 
> complained about that?
> 
> IOW, what compiler are you using and why are only you able to trigger this>

He must be using the pre 6.11-rc2 version of minmax.h

Some of the variables are clearly the wrong type, but min_t() isn't needed
and shouldn't be the fix.
Even the error message tells you to try to fix it differently!

	David

Re: [PATCH] Fix the data type inconsistency issue of min (tier, MAX_CR_TIERS-1) in read_ctrl_pos

[David Laight]

On Fri, 8 Aug 2025 09:35:19 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.08.25 09:21, Qingshuang Fu wrote:
> > From: Qingshuang Fu <fuqingshuang@kylinos.cn>  
> 
> Subject should probably be
> 
> "mm/vmscan: fix build bug in read_ctrl_pos"
> 
> > 
> > Due to the fact that the tier data type in min (tier, MAX_CR_TIERS -1)
> > is int,but MAX_CR_TIERS is an unsigned type, directly using
> > the min function for comparison will result in an error:
> > from mm/vmscan.c:15:
> > mm/vmscan.c: In function ‘read_ctrl_pos’:
> > ./include/linux/build_bug.h:78:41: error: static assertion failed:
> > "min(tier, 4U - 1) signedness error, fix types or
> > consider umin() before min_t()"
> > And MAX_CR_TIERS is a macro definition defined as 4U,
> > so min_t can be used to convert it to int type before
> > performing the minimum value operation.
> >   
> 
> Please use empty lines to make the description easier to read. Also, I 
> think you can simplify this heavily.
> 
> We should add
> 
> Fixes: 37a260870f2c ("mm/mglru: rework type selection")
> 
> BUT
> 
> this commit is more than half a year old. How come no built bot 
> complained about that?
> 
> IOW, what compiler are you using and why are only you able to trigger this>

I've remembered that this code has shown up before.
With the current minmax.h it is ok provided read_ctrl_pos() is inlined.
In that case statically_true((i) >= 0) is true - so the 'signed' 'i' is
known to be non-negative.

But as well as 'i' being signed, the entire loop is silly.
To code is called with a 0..3 to execute the code once (actually in a loop)
or 4 to execute the loop 4 times.
The abstraction is just wrong.

	David