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

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

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?

>0xffffffffffffffffUL results in: 0xffffffffffffffff
>
>
>I thought that "0xffffffff" could be a problem (sign-extending to
>0xffffffffffffffff), but that does not seem to be the case -- likely using
>"unsigned int" as type. Also, I'm surprised that 0xffffffffffffffffU works as
>expected, I would have thought the "U" would make the compiler complain about
>the value not fitting into an unsigned int.
>
>
>When only returning values, the compiler usually does the right thing. Only
>when performing operations on the constant (see ~ example above), we might
>have to use the suffixes, depending on the intended outcome.
>

Looks the guide line is

* no need to put suffix on return value
* add suffix when performing operations, like ~, <<

>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me