Let's remove highmem special-casing from adjust_managed_page_count(),
to result in less confusion why memblock manually adjusts
totalram_pages, and __free_pages_core() only adjusts the zone's
managed pages -- what about the highmem pages that
adjust_managed_page_count() updates?
Now, we only maintain totalram_pages and a zone's managed pages
independent of highmem support. We can derive the number of highmem pages
simply by looking at the relevant zone's managed pages. I don't think
there is any particular fast path that needs a maximum-efficient
totalhigh_pages() implementation.
Note that highmem memory is currently initialized using
free_highmem_page()->free_reserved_page(), not __free_pages_core(). In the
future we might want to also use __free_pages_core() to initialize
highmem memory, to make that less special, and consider moving
totalram_pages updates into __free_pages_core() [1], so we can just use
adjust_managed_page_count() in there as well.
Booting a simple kernel in QEMU reveals no highmem accounting change:
Before:
Memory: 3095448K/3145208K available (14802K kernel code, 2073K rwdata,
5000K rodata, 740K init, 556K bss, 49760K reserved, 0K cma-reserved,
2244488K highmem)
After:
Memory: 3095276K/3145208K available (14802K kernel code, 2073K rwdata,
5000K rodata, 740K init, 556K bss, 49932K reserved, 0K cma-reserved,
2244488K highmem)
[1] https://lkml.kernel.org/r/[email protected]
Cc: Andrew Morton <[email protected]>
Cc: Wei Yang <[email protected]>
David Hildenbrand (2):
mm/highmem: reimplement totalhigh_pages() by walking zones
mm/highmem: make nr_free_highpages() return "unsigned long"
include/linux/highmem-internal.h | 17 ++++++-----------
include/linux/highmem.h | 2 +-
mm/highmem.c | 20 +++++++++++++++-----
mm/page_alloc.c | 4 ----
4 files changed, 22 insertions(+), 21 deletions(-)
base-commit: 19b8422c5bd56fb5e7085995801c6543a98bda1f
--
2.45.1
Can we get rid of the highmem ifdef in adjust_managed_page_count()?
Likely yes: we don't have that many totalhigh_pages() users, and they
all don't seem to be very performance critical.
So let's implement totalhigh_pages() like nr_free_highpages(),
collecting information from all zones. This is now similar to what we do
in si_meminfo_node() to collect the per-node highmem page count.
In the common case (single node, 3-4 zones), we really shouldn't care.
We could optimize a bit further (only walk ZONE_HIGHMEM and ZONE_MOVABLE
if required), but there doesn't seem a real need for that.
Signed-off-by: David Hildenbrand <[email protected]>
---
include/linux/highmem-internal.h | 9 ++-------
mm/highmem.c | 16 +++++++++++++---
mm/page_alloc.c | 4 ----
3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index a3028e400a9c6..65f865fbbac04 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -132,7 +132,7 @@ static inline void __kunmap_atomic(const void *addr)
}
unsigned int __nr_free_highpages(void);
-extern atomic_long_t _totalhigh_pages;
+unsigned long __totalhigh_pages(void);
static inline unsigned int nr_free_highpages(void)
{
@@ -141,12 +141,7 @@ static inline unsigned int nr_free_highpages(void)
static inline unsigned long totalhigh_pages(void)
{
- return (unsigned long)atomic_long_read(&_totalhigh_pages);
-}
-
-static inline void totalhigh_pages_add(long count)
-{
- atomic_long_add(count, &_totalhigh_pages);
+ return __totalhigh_pages();
}
static inline bool is_kmap_addr(const void *x)
diff --git a/mm/highmem.c b/mm/highmem.c
index bd48ba445dd41..3c4e9f8c26dcd 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -111,9 +111,6 @@ static inline wait_queue_head_t *get_pkmap_wait_queue_head(unsigned int color)
}
#endif
-atomic_long_t _totalhigh_pages __read_mostly;
-EXPORT_SYMBOL(_totalhigh_pages);
-
unsigned int __nr_free_highpages(void)
{
struct zone *zone;
@@ -127,6 +124,19 @@ unsigned int __nr_free_highpages(void)
return pages;
}
+unsigned long __totalhigh_pages(void)
+{
+ unsigned long pages = 0;
+ struct zone *zone;
+
+ for_each_populated_zone(zone) {
+ if (is_highmem(zone))
+ pages += zone_managed_pages(zone);
+ }
+
+ return pages;
+}
+
static int pkmap_count[LAST_PKMAP];
static __cacheline_aligned_in_smp DEFINE_SPINLOCK(kmap_lock);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index fc98082a9cf9c..2224965ada468 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5794,10 +5794,6 @@ void adjust_managed_page_count(struct page *page, long count)
{
atomic_long_add(count, &page_zone(page)->managed_pages);
totalram_pages_add(count);
-#ifdef CONFIG_HIGHMEM
- if (PageHighMem(page))
- totalhigh_pages_add(count);
-#endif
}
EXPORT_SYMBOL(adjust_managed_page_count);
--
2.45.1
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 <[email protected]>
---
include/linux/highmem-internal.h | 8 ++++----
include/linux/highmem.h | 2 +-
mm/highmem.c | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index 65f865fbbac04..dd100e849f5e0 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -131,10 +131,10 @@ static inline void __kunmap_atomic(const void *addr)
preempt_enable();
}
-unsigned int __nr_free_highpages(void);
+unsigned long __nr_free_highpages(void);
unsigned long __totalhigh_pages(void);
-static inline unsigned int nr_free_highpages(void)
+static inline unsigned long nr_free_highpages(void)
{
return __nr_free_highpages();
}
@@ -234,8 +234,8 @@ static inline void __kunmap_atomic(const void *addr)
preempt_enable();
}
-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; }
static inline bool is_kmap_addr(const void *x)
{
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 6b0d6f3c8580c..930a591b9b616 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -179,7 +179,7 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
static inline void *kmap_atomic(struct page *page);
/* Highmem related interfaces for management code */
-static inline unsigned int nr_free_highpages(void);
+static inline unsigned long nr_free_highpages(void);
static inline unsigned long totalhigh_pages(void);
#ifndef ARCH_HAS_FLUSH_ANON_PAGE
diff --git a/mm/highmem.c b/mm/highmem.c
index 3c4e9f8c26dcd..1ece1e69031e7 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -111,10 +111,10 @@ static inline wait_queue_head_t *get_pkmap_wait_queue_head(unsigned int color)
}
#endif
-unsigned int __nr_free_highpages(void)
+unsigned long __nr_free_highpages(void)
{
+ unsigned long pages = 0;
struct zone *zone;
- unsigned int pages = 0;
for_each_populated_zone(zone) {
if (is_highmem(zone))
--
2.45.1
On 07.06.24 10:37, David Hildenbrand wrote:
> Can we get rid of the highmem ifdef in adjust_managed_page_count()?
> Likely yes: we don't have that many totalhigh_pages() users, and they
> all don't seem to be very performance critical.
>
> So let's implement totalhigh_pages() like nr_free_highpages(),
> collecting information from all zones. This is now similar to what we do
> in si_meminfo_node() to collect the per-node highmem page count.
>
> In the common case (single node, 3-4 zones), we really shouldn't care.
> We could optimize a bit further (only walk ZONE_HIGHMEM and ZONE_MOVABLE
> if required), but there doesn't seem a real need for that.
>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> include/linux/highmem-internal.h | 9 ++-------
> mm/highmem.c | 16 +++++++++++++---
> mm/page_alloc.c | 4 ----
> 3 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
> index a3028e400a9c6..65f865fbbac04 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -132,7 +132,7 @@ static inline void __kunmap_atomic(const void *addr)
> }
>
> unsigned int __nr_free_highpages(void);
> -extern atomic_long_t _totalhigh_pages;
> +unsigned long __totalhigh_pages(void);
>
> static inline unsigned int nr_free_highpages(void)
> {
> @@ -141,12 +141,7 @@ static inline unsigned int nr_free_highpages(void)
>
> static inline unsigned long totalhigh_pages(void)
> {
> - return (unsigned long)atomic_long_read(&_totalhigh_pages);
> -}
> -
> -static inline void totalhigh_pages_add(long count)
> -{
> - atomic_long_add(count, &_totalhigh_pages);
> + return __totalhigh_pages();
> }
>
> static inline bool is_kmap_addr(const void *x)
> diff --git a/mm/highmem.c b/mm/highmem.c
> index bd48ba445dd41..3c4e9f8c26dcd 100644
> --- a/mm/highmem.c
> +++ b/mm/highmem.c
> @@ -111,9 +111,6 @@ static inline wait_queue_head_t *get_pkmap_wait_queue_head(unsigned int color)
> }
> #endif
>
> -atomic_long_t _totalhigh_pages __read_mostly;
> -EXPORT_SYMBOL(_totalhigh_pages);
> -
> unsigned int __nr_free_highpages(void)
> {
> struct zone *zone;
> @@ -127,6 +124,19 @@ unsigned int __nr_free_highpages(void)
> return pages;
> }
>
> +unsigned long __totalhigh_pages(void)
> +{
> + unsigned long pages = 0;
> + struct zone *zone;
> +
> + for_each_populated_zone(zone) {
> + if (is_highmem(zone))
> + pages += zone_managed_pages(zone);
> + }
> +
> + return pages;
> +}
> +
Another (later than usual, but better late than never) build bot complaint:
diff --git a/mm/highmem.c b/mm/highmem.c
index 3c4e9f8c26dcd..b14771fb05ed1 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -136,6 +136,7 @@ unsigned long __totalhigh_pages(void)
return pages;
}
+EXPORT_SYMBOL(__totalhigh_pages);
static int pkmap_count[LAST_PKMAP];
static __cacheline_aligned_in_smp DEFINE_SPINLOCK(kmap_lock);
--
Cheers,
David / dhildenb
On Fri, Jun 07, 2024 at 10:37:09AM +0200, David Hildenbrand wrote:
>Let's remove highmem special-casing from adjust_managed_page_count(),
>to result in less confusion why memblock manually adjusts
>totalram_pages, and __free_pages_core() only adjusts the zone's
>managed pages -- what about the highmem pages that
>adjust_managed_page_count() updates?
>
Thanks David
I have looked into this function and willing to get rid of it, but not found a
good way.
Your change really look nice.
>Now, we only maintain totalram_pages and a zone's managed pages
>independent of highmem support. We can derive the number of highmem pages
>simply by looking at the relevant zone's managed pages. I don't think
>there is any particular fast path that needs a maximum-efficient
>totalhigh_pages() implementation.
>
>Note that highmem memory is currently initialized using
>free_highmem_page()->free_reserved_page(), not __free_pages_core(). In the
>future we might want to also use __free_pages_core() to initialize
>highmem memory, to make that less special, and consider moving
>totalram_pages updates into __free_pages_core() [1], so we can just use
>adjust_managed_page_count() in there as well.
>
>Booting a simple kernel in QEMU reveals no highmem accounting change:
>
>Before:
> Memory: 3095448K/3145208K available (14802K kernel code, 2073K rwdata,
> 5000K rodata, 740K init, 556K bss, 49760K reserved, 0K cma-reserved,
> 2244488K highmem)
>
>After:
> Memory: 3095276K/3145208K available (14802K kernel code, 2073K rwdata,
> 5000K rodata, 740K init, 556K bss, 49932K reserved, 0K cma-reserved,
> 2244488K highmem)
>
>[1] https://lkml.kernel.org/r/[email protected]
>
>Cc: Andrew Morton <[email protected]>
>Cc: Wei Yang <[email protected]>
>
>David Hildenbrand (2):
> mm/highmem: reimplement totalhigh_pages() by walking zones
> mm/highmem: make nr_free_highpages() return "unsigned long"
>
> include/linux/highmem-internal.h | 17 ++++++-----------
> include/linux/highmem.h | 2 +-
> mm/highmem.c | 20 +++++++++++++++-----
> mm/page_alloc.c | 4 ----
> 4 files changed, 22 insertions(+), 21 deletions(-)
>
>
>base-commit: 19b8422c5bd56fb5e7085995801c6543a98bda1f
>--
>2.45.1
--
Wei Yang
Help you, Help me
On Fri, Jun 07, 2024 at 10:37:10AM +0200, David Hildenbrand wrote:
>Can we get rid of the highmem ifdef in adjust_managed_page_count()?
>Likely yes: we don't have that many totalhigh_pages() users, and they
>all don't seem to be very performance critical.
>
>So let's implement totalhigh_pages() like nr_free_highpages(),
>collecting information from all zones. This is now similar to what we do
>in si_meminfo_node() to collect the per-node highmem page count.
>
>In the common case (single node, 3-4 zones), we really shouldn't care.
>We could optimize a bit further (only walk ZONE_HIGHMEM and ZONE_MOVABLE
>if required), but there doesn't seem a real need for that.
>
>Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Wei Yang <[email protected]>
>---
> include/linux/highmem-internal.h | 9 ++-------
> mm/highmem.c | 16 +++++++++++++---
> mm/page_alloc.c | 4 ----
> 3 files changed, 15 insertions(+), 14 deletions(-)
>
>diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
>index a3028e400a9c6..65f865fbbac04 100644
>--- a/include/linux/highmem-internal.h
>+++ b/include/linux/highmem-internal.h
>@@ -132,7 +132,7 @@ static inline void __kunmap_atomic(const void *addr)
> }
>
> unsigned int __nr_free_highpages(void);
>-extern atomic_long_t _totalhigh_pages;
>+unsigned long __totalhigh_pages(void);
>
> static inline unsigned int nr_free_highpages(void)
> {
>@@ -141,12 +141,7 @@ static inline unsigned int nr_free_highpages(void)
>
> static inline unsigned long totalhigh_pages(void)
> {
>- return (unsigned long)atomic_long_read(&_totalhigh_pages);
>-}
>-
>-static inline void totalhigh_pages_add(long count)
>-{
>- atomic_long_add(count, &_totalhigh_pages);
>+ return __totalhigh_pages();
> }
>
> static inline bool is_kmap_addr(const void *x)
>diff --git a/mm/highmem.c b/mm/highmem.c
>index bd48ba445dd41..3c4e9f8c26dcd 100644
>--- a/mm/highmem.c
>+++ b/mm/highmem.c
>@@ -111,9 +111,6 @@ static inline wait_queue_head_t *get_pkmap_wait_queue_head(unsigned int color)
> }
> #endif
>
>-atomic_long_t _totalhigh_pages __read_mostly;
>-EXPORT_SYMBOL(_totalhigh_pages);
>-
> unsigned int __nr_free_highpages(void)
> {
> struct zone *zone;
>@@ -127,6 +124,19 @@ unsigned int __nr_free_highpages(void)
> return pages;
> }
>
>+unsigned long __totalhigh_pages(void)
>+{
>+ unsigned long pages = 0;
>+ struct zone *zone;
>+
>+ for_each_populated_zone(zone) {
>+ if (is_highmem(zone))
>+ pages += zone_managed_pages(zone);
>+ }
>+
>+ return pages;
>+}
>+
> static int pkmap_count[LAST_PKMAP];
> static __cacheline_aligned_in_smp DEFINE_SPINLOCK(kmap_lock);
>
>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>index fc98082a9cf9c..2224965ada468 100644
>--- a/mm/page_alloc.c
>+++ b/mm/page_alloc.c
>@@ -5794,10 +5794,6 @@ void adjust_managed_page_count(struct page *page, long count)
> {
> atomic_long_add(count, &page_zone(page)->managed_pages);
> totalram_pages_add(count);
>-#ifdef CONFIG_HIGHMEM
>- if (PageHighMem(page))
>- totalhigh_pages_add(count);
>-#endif
> }
> EXPORT_SYMBOL(adjust_managed_page_count);
>
>--
>2.45.1
--
Wei Yang
Help you, Help me
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.
I am not sure why not use 0UL for both?
>
>Signed-off-by: David Hildenbrand <[email protected]>
>---
> include/linux/highmem-internal.h | 8 ++++----
> include/linux/highmem.h | 2 +-
> mm/highmem.c | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
>index 65f865fbbac04..dd100e849f5e0 100644
>--- a/include/linux/highmem-internal.h
>+++ b/include/linux/highmem-internal.h
>@@ -131,10 +131,10 @@ static inline void __kunmap_atomic(const void *addr)
> preempt_enable();
> }
>
>-unsigned int __nr_free_highpages(void);
>+unsigned long __nr_free_highpages(void);
> unsigned long __totalhigh_pages(void);
>
>-static inline unsigned int nr_free_highpages(void)
>+static inline unsigned long nr_free_highpages(void)
> {
> return __nr_free_highpages();
> }
>@@ -234,8 +234,8 @@ static inline void __kunmap_atomic(const void *addr)
> preempt_enable();
> }
>
>-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; }
>
> static inline bool is_kmap_addr(const void *x)
> {
>diff --git a/include/linux/highmem.h b/include/linux/highmem.h
>index 6b0d6f3c8580c..930a591b9b616 100644
>--- a/include/linux/highmem.h
>+++ b/include/linux/highmem.h
>@@ -179,7 +179,7 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
> static inline void *kmap_atomic(struct page *page);
>
> /* Highmem related interfaces for management code */
>-static inline unsigned int nr_free_highpages(void);
>+static inline unsigned long nr_free_highpages(void);
> static inline unsigned long totalhigh_pages(void);
>
> #ifndef ARCH_HAS_FLUSH_ANON_PAGE
>diff --git a/mm/highmem.c b/mm/highmem.c
>index 3c4e9f8c26dcd..1ece1e69031e7 100644
>--- a/mm/highmem.c
>+++ b/mm/highmem.c
>@@ -111,10 +111,10 @@ static inline wait_queue_head_t *get_pkmap_wait_queue_head(unsigned int color)
> }
> #endif
>
>-unsigned int __nr_free_highpages(void)
>+unsigned long __nr_free_highpages(void)
> {
>+ unsigned long pages = 0;
> struct zone *zone;
>- unsigned int pages = 0;
>
> for_each_populated_zone(zone) {
> if (is_highmem(zone))
>--
>2.45.1
--
Wei Yang
Help you, Help me
On Fri, Jun 07, 2024 at 10:37:10AM +0200, David Hildenbrand wrote:
> Can we get rid of the highmem ifdef in adjust_managed_page_count()?
> Likely yes: we don't have that many totalhigh_pages() users, and they
> all don't seem to be very performance critical.
>
> So let's implement totalhigh_pages() like nr_free_highpages(),
> collecting information from all zones. This is now similar to what we do
> in si_meminfo_node() to collect the per-node highmem page count.
>
> In the common case (single node, 3-4 zones), we really shouldn't care.
> We could optimize a bit further (only walk ZONE_HIGHMEM and ZONE_MOVABLE
> if required), but there doesn't seem a real need for that.
>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Oscar Salvador <[email protected]>
--
Oscar Salvador
SUSE Labs
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 <[email protected]>
...
> -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.
Other than that
Reviewed-by: Oscar Salvador <[email protected]>
--
Oscar Salvador
SUSE Labs
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 <[email protected]>
> ...
>> -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;"
So I prefer to just drop it.
Thanks!
--
Cheers,
David / dhildenb
On 08.06.24 02:51, Wei Yang 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.
>
> I am not sure why not use 0UL for both?
See my reply to Oscar, thanks for the review!
--
Cheers,
David / dhildenb
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 <[email protected]>
>> ...
>> > -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
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 <[email protected]>
>>> ...
>>>> -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
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.
--
Cheers,
David / dhildenb
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 <[email protected]>
>> > > ...
>> > > > -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
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 <[email protected]>
>>>>> ...
>>>>>> -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
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 <[email protected]>
>> > > > > ...
>> > > > > > -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 <[email protected]>
>--
>Cheers,
>
>David / dhildenb
--
Wei Yang
Help you, Help me