Re: [PATCH v1 2/2] mm/highmem: make nr_free_highpages() return "unsigned long"

[Wei Yang <richard.weiyang@gmail.com>]

On Wed, Jun 12, 2024 at 09:22:25AM +0200, David Hildenbrand wrote:
>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@redhat.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.
>

Interesting, thanks for the reference.

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>

>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me