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

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

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?

>
>So I prefer to just drop it.
>
>Thanks!
>
>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me