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

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

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 

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

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)

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?

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.

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