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

[fffsqian <fffsqian@163.com>]

[-- 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 --]