To the compiler, (MAX_NR_TIERS - 1) (i.e., (4U - 1)) is unsigned,
whereas tier is a signed integer.

GCC 5.4 does not permit the minimum operation on such
type-inconsistent operands.

Cast it to a signed integer to circumvent this compiler error.

Fix follow error with gcc 5.4:
  mm/vmscan.c: In function ‘read_ctrl_pos’:
  mm/vmscan.c:3166:728: error: call to ‘__compiletime_assert_887’ declared with attribute error: min(tier, 4U - 1) signedness error

Fixes: 37a260870f2c ("mm/mglru: rework type selection")
Signed-off-by: WangYuli <wangyuli@uniontech.com>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3783e45bfc92..29dce1aed962 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3163,7 +3163,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(tier, (int)(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.49.0

On Wed,  7 May 2025 00:02:38 +0800 WangYuli <wangyuli@uniontech.com> wrote:

> -	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
> +	for (i = tier % MAX_NR_TIERS; i <= min(tier, (int)(MAX_NR_TIERS - 1)); i++) {
	
Make `tier' unsigned?  After all, negative tier numbers are nonsensical.

Hi Andrew Morton,

On 2025/5/7 07:24, Andrew Morton wrote:
> On Wed,  7 May 2025 00:02:38 +0800 WangYuli <wangyuli@uniontech.com> wrote:
>
> Make `tier' unsigned?  After all, negative tier numbers are nonsensical.

That point is well taken.

However, I've noticed that variables named "tier" seem to be commonly 
defined as int rather than unsigned int throughout the mm subsystem, and 
perhaps even the wider kernel code.

I was wondering if changing just this one instance might feel a little 
inconsistent?

Perhaps a possible approach for now could be to change this line to for 
(i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) 
{, which would allow us to keep the signed int type for the tier 
variable itself.

Regarding the potential for a more comprehensive change in the future to 
redefine all these "tier" variables and related ones as unsigned int, I 
would be very grateful for your guidance on whether that's a direction 
we should consider.

But actually, whether it's signed or not likely won't affect its normal 
operation...

Thanks,

-- 
WangYuli

On Wed, 7 May 2025 12:06:11 +0800
WangYuli <wangyuli@uniontech.com> wrote:

> Hi Andrew Morton,
> 
> On 2025/5/7 07:24, Andrew Morton wrote:
> > On Wed,  7 May 2025 00:02:38 +0800 WangYuli <wangyuli@uniontech.com> wrote:
> >
> > Make `tier' unsigned?  After all, negative tier numbers are nonsensical.  
> 
> That point is well taken.
> 
> However, I've noticed that variables named "tier" seem to be commonly 
> defined as int rather than unsigned int throughout the mm subsystem, and 
> perhaps even the wider kernel code.
> 
> I was wondering if changing just this one instance might feel a little 
> inconsistent?
> 
> Perhaps a possible approach for now could be to change this line to for 
> (i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) 
> {, which would allow us to keep the signed int type for the tier 
> variable itself.

Remember that min_t(int, a, b) is just min((int)a, (int)b) and you really
wouldn't put casts like that in code.
Even if a cast can't be avoided only one side would normally need it.

There really ought to be a 'duck shoot' against min_t().
I'm trying very hard to stop any more being added.

	David

> 
> Regarding the potential for a more comprehensive change in the future to 
> redefine all these "tier" variables and related ones as unsigned int, I 
> would be very grateful for your guidance on whether that's a direction 
> we should consider.
> 
> But actually, whether it's signed or not likely won't affect its normal 
> operation...
> 
> Thanks,
>

Hi all,

Apologies for the delayed response.

First, thank you all for your reminders which helped me understand 
exactly what caused the GCC 5.4 compile error, the proper fix, and the 
issue description.

However, this change might be unnecessary now.

Thanks to Nathan Chancellor's note in another email that the minimum 
version of GCC is being bumped to 8.1.[1]

My testing confirms that GCC 8.1 no longer exhibits this issue.

[1]. https://lore.kernel.org/all/20250508163138.GA834338@ax162/

Thanks,
-- 
WangYuli

On Wed, 7 May 2025 12:06:11 +0800 WangYuli <wangyuli@uniontech.com> wrote:

> Perhaps a possible approach for now could be to change this line to for 
> (i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) 
> {, which would allow us to keep the signed int type for the tier 
> variable itself.

Sure.

> Regarding the potential for a more comprehensive change in the future to 
> redefine all these "tier" variables and related ones as unsigned int, I 
> would be very grateful for your guidance on whether that's a direction 
> we should consider.

`int' is a curse.  Yes, we do this very frequently - we unthinkingly
use a signed type for a naturally unsigned concept.  Ths signed type
spreads and spreads and the incorrect signage causes (small) problems
in various places.  By then it's a big mess trying to switch to an
unsigned type.

Oh well, we just battle on.  We should at least be more vigilant about
this when adding new things.


hp2:/usr/src/25> grep "int nid" mm/*.c | wc -l
316

argh.

On Wed, May 07, 2025 at 11:07:01AM -0700, Andrew Morton wrote:
> `int' is a curse.  Yes, we do this very frequently - we unthinkingly
> use a signed type for a naturally unsigned concept.  Ths signed type
> spreads and spreads and the incorrect signage causes (small) problems
> in various places.  By then it's a big mess trying to switch to an
> unsigned type.
> 
> Oh well, we just battle on.  We should at least be more vigilant about
> this when adding new things.
> 
> 
> hp2:/usr/src/25> grep "int nid" mm/*.c | wc -l
> 316

$ git grep def.*NUMA_NO_NODE include
include/linux/nodemask_types.h:#define  NUMA_NO_NODE    (-1)

I bet if you change all of those to unsigned, you'll get a
non-functional kernel.  C is awful, we need to dump it.

On Wed, May 07, 2025 at 12:02:38AM +0800, WangYuli wrote:
> To the compiler, (MAX_NR_TIERS - 1) (i.e., (4U - 1)) is unsigned,
> whereas tier is a signed integer.
> 
> GCC 5.4 does not permit the minimum operation on such
> type-inconsistent operands.

1. This has nothing to do with the compiler version; the type-checking
is built into min().
2. We have min_t for a reason
3. Why is a signed min the right answer instead of an unsigned min?

On Tue, 6 May 2025 17:22:51 +0100
Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, May 07, 2025 at 12:02:38AM +0800, WangYuli wrote:
> > To the compiler, (MAX_NR_TIERS - 1) (i.e., (4U - 1)) is unsigned,
> > whereas tier is a signed integer.
> > 
> > GCC 5.4 does not permit the minimum operation on such
> > type-inconsistent operands.  
> 
> 1. This has nothing to do with the compiler version; the type-checking
> is built into min().
> 2. We have min_t for a reason

Mostly historical - to match the original inline function min().
min_t() is definitely overused, it should be the 'last resort'
for a type mismatch, not the first.

> 3. Why is a signed min the right answer instead of an unsigned min?
> 

I don't seem to have the patch itself, but I' guessing it is for:

for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {

which seems to have been added for 6.14-rc1 - so why is it only an issue now.

Looks closer, I bet the function is usually inlined.

	David

Hi Matthew Wilcox and Andrew Morton,

On 2025/5/7 00:22, Matthew Wilcox wrote:
> 1. This has nothing to do with the compiler version; the type-checking
> is built into min().

Thank you for pointing that out! My previous statement was incorrect.

The error is actually from the __types_ok check within the 
__careful_cmp_once macro failing, which triggered BUILD_BUG_ON.

But then, why do newer compilers compile this without error? Is it 
perhaps because they consider 4U - 1 to be signed?

> 2. We have min_t for a reason
Yes, using min_t instead of min is a better approach.
> 3. Why is a signed min the right answer instead of an unsigned min?
>
That is indeed false. Just as Andrew Morton said, "negative tier numbers 
are nonsensical".

Thank you for everyone's corrections. I'll submit a v2 patch.

-- 
WangYuli

On Wed, 7 May 2025 10:55:31 +0800
WangYuli <wangyuli@uniontech.com> wrote:

> Hi Matthew Wilcox and Andrew Morton,
> 
> On 2025/5/7 00:22, Matthew Wilcox wrote:
> > 1. This has nothing to do with the compiler version; the type-checking
> > is built into min().  
> 
> Thank you for pointing that out! My previous statement was incorrect.
> 
> The error is actually from the __types_ok check within the 
> __careful_cmp_once macro failing, which triggered BUILD_BUG_ON.
> 
> But then, why do newer compilers compile this without error? Is it 
> perhaps because they consider 4U - 1 to be signed?

No, statically_true(tier >= 0) is true even though 'tier' is signed
because the compiler has inlined the function and followed the domain
of 'tier' from the assignments and loops.

	David