Date: Wed, 12 Jun 2024 09:22:25 +0200
From: David Hildenbrand <david@...hat.com>
To: Wei Yang <richard.weiyang@...il.com>
Cc: Oscar Salvador <osalvador@...e.de>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v1 2/2] mm/highmem: make nr_free_highpages() return
 "unsigned long"

On 12.06.24 09:01, Wei Yang wrote:
> On Tue, Jun 11, 2024 at 11:20:00AM +0200, David Hildenbrand wrote:
>> On 11.06.24 02:56, Wei Yang wrote:
>>> On Mon, Jun 10, 2024 at 10:22:49AM +0200, David Hildenbrand wrote:
>>>> On 10.06.24 05:40, Oscar Salvador wrote:
>>>>> On Fri, Jun 07, 2024 at 10:37:11AM +0200, David Hildenbrand wrote:
>>>>>> It looks rather weird that totalhigh_pages() returns an
>>>>>> "unsigned long" but nr_free_highpages() returns an "unsigned int".
>>>>>>
>>>>>> Let's return an "unsigned long" from nr_free_highpages() to be
>>>>>> consistent.
>>>>>>
>>>>>> While at it, use a plain "0" instead of a "0UL" in the !CONFIG_HIGHMEM
>>>>>> totalhigh_pages() implementation, to make these look alike as well.
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <david@...hat.com>
>>>>> ...
>>>>>> -static inline unsigned int nr_free_highpages(void) { return 0; }
>>>>>> -static inline unsigned long totalhigh_pages(void) { return 0UL; }
>>>>>> +static inline unsigned long nr_free_highpages(void) { return 0; }
>>>>>> +static inline unsigned long totalhigh_pages(void) { return 0; }
>>>>>
>>>>> Although I doubt it has any consequences, I would just leave them both with UL,
>>>>> so the return type is consistent with what we are returning.
>>>>
>>>> These suffixes are only required when using constants that would not fit
>>>> into the native (int) type, or converting from that native (int) type to
>>>> something else automatically by the compiler would mess things up (for example,
>>>> undesired sign extension). For 0 that is certainly impossible :)
>>>>
>>>>
>>>> That's also the reason why in include/linux we now have:
>>>>
>>>> t14s: ~/git/linux/include/linux $ git grep "return 0UL;"
>>>> skbuff.h:       return 0UL;
>>>> uaccess.h:static inline unsigned long user_access_save(void) { return 0UL; }
>>>> t14s: ~/git/linux/include/linux $ git grep "0UL;"
>>>> bitmap.h:               *dst = ~0UL;
>>>> dax.h:          return ~0UL;
>>>> mtd/map.h:                      r.x[i] = ~0UL;
>>>> netfilter.h:    return ((ul1[0] ^ ul2[0]) | (ul1[1] ^ ul2[1])) == 0UL;
>>>> skbuff.h:       return 0UL;
>>>> uaccess.h:static inline unsigned long user_access_save(void) { return 0UL; }
>>>>
>>>>
>>>> ... compared to a long list if "unsigned long" functions that simply "return 0;"
>>>>
>>>
>>> Seems this is the current status.
>>>
>>> Then my question is do we have a guide line for this? Or 0 is the special
>>> case? Sounds positive value has no sign extension problem. If we need to
>>> return 1, we suppose to use 1 or 1UL? I found myself confused.
>>>
>>> I grepped "return 1" and do find some cases without UL:
>>>
>>> backing-dev.h: wb_stat_error() return 1 for unsigned long.
>>> pgtable.h: pte_batch_hint() return 1 for unsigned int.
>>>
>>> So the guide line is for positive value, it is not necessary to use UL?
>>
>> I think when returning simple values (0/1/-1), we really don't need these
>> suffices at all. The standard says "The type of an integer constant is the
>> first of the corresponding list in which its value can be represented.". I
>> thought it would always use an "int", but that is not the case.
>>
>> So, if we use "-1", the compiler will use an "int", and sign extension to
>> "unsigned" long will do the right thing.
>>
>> Simple test:
>>
>> -1 results in: 0xffffffffffffffff
>> -1U results in: 0xffffffff
>> -1UL results in: 0xffffffffffffffff
>> 0xffffffff results in: 0xffffffff
>> 0xffffffffU results in: 0xffffffff
>> 0xffffffffUL results in: 0xffffffff
>> ~0xffffffff results in: 0x0
>> ~0xffffffffU results in: 0x0
>> ~0xffffffffUL results in: 0xffffffff00000000
>> 0xffffffffffffffff results in: 0xffffffffffffffff
>> 0xffffffffffffffffU results in: 0xffffffffffffffff
> 
> I expect this to be 0xffffffff. Why this extend it to a UL?

Apparently, the "U" only restricts the set of types to "unsigned ones".

https://en.cppreference.com/w/cpp/language/integer_literal

So the compiler will use the first "unsigned" type that can hold that value.

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists