Received-SPF: pass (google.com: domain of linux-kernel+bounces-209139-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 11 Jun 2024 00:56:36 +0000
From: Wei Yang <richard.weiyang@gmail.com>
To: David Hildenbrand <david@redhat.com>
Cc: Oscar Salvador <osalvador@suse.de>, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	Wei Yang <richard.weiyang@gmail.com>
Subject: Re: [PATCH v1 2/2] mm/highmem: make nr_free_highpages() return
 "unsigned long"
Message-ID: <20240611005636.g6525rkqpos43yds@master>
Reply-To: Wei Yang <richard.weiyang@gmail.com>
References: <20240607083711.62833-1-david@redhat.com>
 <20240607083711.62833-3-david@redhat.com>
 <ZmZ1x6QQYPFSOd7O@localhost.localdomain>
 <99073d55-5b18-4ed2-b860-8c09e056f585@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <99073d55-5b18-4ed2-b860-8c09e056f585@redhat.com>
User-Agent: NeoMutt/20170113 (1.7.2)

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