[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

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

[fffsqian]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

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.




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.




Thanks again for your help!




Qingshuang Fu fffsqian@163.com

















At 2025-08-08 15:35:19, "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>
>
>> 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

[-- Attachment #2: Type: text/html, Size: 4261 bytes --]

[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