Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

2024-04-26 Thread Jan Beulich 
On 26.04.2024 14:09, Oleksii wrote:
> On Fri, 2024-04-26 at 12:51 +0200, Jan Beulich wrote:
>> On 26.04.2024 10:21, Oleksii wrote:
>>> On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
 On 17.04.2024 12:04, Oleksii Kurochko wrote:
> Return type was left 'int' because of the following compilation
> error:
>
> ./include/xen/kernel.h:18:21: error: comparison of distinct
> pointer
> types lacks a cast [-Werror]
>    18 | (void) (&_x == &_y);    \
>   | ^~
>     common/page_alloc.c:1843:34: note: in expansion of macro
> 'min'
>  1843 | unsigned int inc_order = min(MAX_ORDER,
> flsl(e
> - s) - 1);

 Apart from this I'm okay with this patch, assuming Andrew's won't
 change in
 any conflicting way. As to the above - no, I don't see us having
 ffs() / ffsl()
 returning unsigned int, fls() / flsl() returning plain int. Even
 more
 so that,
 given the LHS variable's type, an unsigned quantity is really
 meant
 in the
 quoted code.
>>> If I understand you correctly, it's acceptable for fls() / flsl()
>>> to
>>> return 'int'. Therefore, I can update the commit message by
>>> removing
>>> the part mentioning the compilation error, as it's expected for
>>> fls() /
>>> flsl() to return 'int'. Is my understanding correct?
>>
>> No. I firmly object to ffs() and fls() being different in their
>> return
>> types. I'm sorry, I realize now that my earlier wording was ambiguous
>> (at least missing "but" after the comma).
> Thanks for clarifying.
> 
> I can change return type of fls() / flsl() to 'unsingned int' to be the
> same as return type of ffs() / ffsl(), but then it will be needed to
> add a cast in two places:

Except that no, it doesn't really need casts there.

>--- a/xen/common/page_alloc.c
>+++ b/xen/common/page_alloc.c
>@@ -1842,7 +1842,7 @@ static void _init_heap_pages(const struct
>page_info *pg,
>  * Note that the value of ffsl() and flsl() starts from 1
>so we need
>  * to decrement it by 1.
>  */
>-unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);
>+unsigned int inc_order = min((unsigned int)MAX_ORDER,
>flsl(e - s) - 1);

The preferred course of action would want to be to simply make MAX_ORDER
expand to an unsigned constant. Depending on the amount of fallout, an
alternative would be to use _AC(MAX_ORDER, U) here. Yet another
alternative would be to use MAX_ORDER + 0U here, as iirc we do in a few
other places, for similar purposes.

Avoiding a cast here is not only shorter, but - see statements elsewhere -
generally preferable.

Jan

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

2024-04-26 Thread Oleksii 
On Fri, 2024-04-26 at 12:51 +0200, Jan Beulich wrote:
> On 26.04.2024 10:21, Oleksii wrote:
> > On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
> > > On 17.04.2024 12:04, Oleksii Kurochko wrote:
> > > > Return type was left 'int' because of the following compilation
> > > > error:
> > > > 
> > > > ./include/xen/kernel.h:18:21: error: comparison of distinct
> > > > pointer
> > > > types lacks a cast [-Werror]
> > > >    18 | (void) (&_x == &_y);    \
> > > >   | ^~
> > > >     common/page_alloc.c:1843:34: note: in expansion of macro
> > > > 'min'
> > > >  1843 | unsigned int inc_order = min(MAX_ORDER,
> > > > flsl(e
> > > > - s) - 1);
> > > 
> > > Apart from this I'm okay with this patch, assuming Andrew's won't
> > > change in
> > > any conflicting way. As to the above - no, I don't see us having
> > > ffs() / ffsl()
> > > returning unsigned int, fls() / flsl() returning plain int. Even
> > > more
> > > so that,
> > > given the LHS variable's type, an unsigned quantity is really
> > > meant
> > > in the
> > > quoted code.
> > If I understand you correctly, it's acceptable for fls() / flsl()
> > to
> > return 'int'. Therefore, I can update the commit message by
> > removing
> > the part mentioning the compilation error, as it's expected for
> > fls() /
> > flsl() to return 'int'. Is my understanding correct?
> 
> No. I firmly object to ffs() and fls() being different in their
> return
> types. I'm sorry, I realize now that my earlier wording was ambiguous
> (at least missing "but" after the comma).
Thanks for clarifying.

I can change return type of fls() / flsl() to 'unsingned int' to be the
same as return type of ffs() / ffsl(), but then it will be needed to
add a cast in two places:
   --- a/xen/common/page_alloc.c
   +++ b/xen/common/page_alloc.c
   @@ -1842,7 +1842,7 @@ static void _init_heap_pages(const struct
   page_info *pg,
 * Note that the value of ffsl() and flsl() starts from 1
   so we need
 * to decrement it by 1.
 */
   -unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);
   +unsigned int inc_order = min((unsigned int)MAX_ORDER,
   flsl(e - s) - 1);

if ( s )
inc_order = min(inc_order, ffsl(s) - 1U);
   @@ -2266,7 +2266,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
ASSERT(!first_node_initialised);
ASSERT(!xenheap_bits);
BUILD_BUG_ON((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG);
   -xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
   +xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, (unsigned
   int)PADDR_BITS);
printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
}
   
If it looks okay, then I'll do that in the next patch version.

~ Oleksii

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

2024-04-26 Thread Jan Beulich 
On 26.04.2024 10:21, Oleksii wrote:
> On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
>> On 17.04.2024 12:04, Oleksii Kurochko wrote:
>>> Return type was left 'int' because of the following compilation
>>> error:
>>>
>>> ./include/xen/kernel.h:18:21: error: comparison of distinct pointer
>>> types lacks a cast [-Werror]
>>>    18 | (void) (&_x == &_y);    \
>>>   | ^~
>>>     common/page_alloc.c:1843:34: note: in expansion of macro 'min'
>>>  1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e
>>> - s) - 1);
>>
>> Apart from this I'm okay with this patch, assuming Andrew's won't
>> change in
>> any conflicting way. As to the above - no, I don't see us having
>> ffs() / ffsl()
>> returning unsigned int, fls() / flsl() returning plain int. Even more
>> so that,
>> given the LHS variable's type, an unsigned quantity is really meant
>> in the
>> quoted code.
> If I understand you correctly, it's acceptable for fls() / flsl() to
> return 'int'. Therefore, I can update the commit message by removing
> the part mentioning the compilation error, as it's expected for fls() /
> flsl() to return 'int'. Is my understanding correct?

No. I firmly object to ffs() and fls() being different in their return
types. I'm sorry, I realize now that my earlier wording was ambiguous
(at least missing "but" after the comma).

Jan

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

2024-04-26 Thread Oleksii 
On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
> On 17.04.2024 12:04, Oleksii Kurochko wrote:
> > Return type was left 'int' because of the following compilation
> > error:
> > 
> > ./include/xen/kernel.h:18:21: error: comparison of distinct pointer
> > types lacks a cast [-Werror]
> >    18 | (void) (&_x == &_y);    \
> >   | ^~
> >     common/page_alloc.c:1843:34: note: in expansion of macro 'min'
> >  1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e
> > - s) - 1);
> 
> Apart from this I'm okay with this patch, assuming Andrew's won't
> change in
> any conflicting way. As to the above - no, I don't see us having
> ffs() / ffsl()
> returning unsigned int, fls() / flsl() returning plain int. Even more
> so that,
> given the LHS variable's type, an unsigned quantity is really meant
> in the
> quoted code.
If I understand you correctly, it's acceptable for fls() / flsl() to
return 'int'. Therefore, I can update the commit message by removing
the part mentioning the compilation error, as it's expected for fls() /
flsl() to return 'int'. Is my understanding correct?

~ Oleksii

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

2024-04-25 Thread Jan Beulich 
On 17.04.2024 12:04, Oleksii Kurochko wrote:
> Return type was left 'int' because of the following compilation error:
> 
> ./include/xen/kernel.h:18:21: error: comparison of distinct pointer types 
> lacks a cast [-Werror]
>18 | (void) (&_x == &_y);\
>   | ^~
> common/page_alloc.c:1843:34: note: in expansion of macro 'min'
>  1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);

Apart from this I'm okay with this patch, assuming Andrew's won't change in
any conflicting way. As to the above - no, I don't see us having ffs() / ffsl()
returning unsigned int, fls() / flsl() returning plain int. Even more so that,
given the LHS variable's type, an unsigned quantity is really meant in the
quoted code.

Jan

[PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

2024-04-17 Thread Oleksii Kurochko 
Return type was left 'int' because of the following compilation error:

./include/xen/kernel.h:18:21: error: comparison of distinct pointer types lacks 
a cast [-Werror]
   18 | (void) (&_x == &_y);\
  | ^~
common/page_alloc.c:1843:34: note: in expansion of macro 'min'
 1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);

generic_fls{l} was used instead of __builtin_clz{l}(x) as if x is 0,
the result in undefined.

Signed-off-by: Oleksii Kurochko 
---
 The patch is almost independent from Andrew's patch series
 ( 
https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.coop...@citrix.com/T/#t)
 except test_fls() function which IMO can be merged as a separate patch after 
Andrew's patch
 will be fully ready.
---
Changes in V8:
 - do proper rebase: back definition of fls{l} to the current patch.
 - drop the changes which removed ffz() in PPC. it should be done not
   in this patch.
 - add a message after Signed-off.
---
Changes in V7:
 - Code style fixes
---
Changes in V6:
 - new patch for the patch series.
---
 xen/arch/arm/include/asm/arm32/bitops.h |  2 +-
 xen/arch/arm/include/asm/arm64/bitops.h |  6 ++
 xen/arch/arm/include/asm/bitops.h   |  7 ++-
 xen/arch/ppc/include/asm/bitops.h   |  3 ---
 xen/arch/x86/include/asm/bitops.h   |  6 --
 xen/common/bitops.c | 22 ++
 xen/include/xen/bitops.h| 24 
 7 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm32/bitops.h 
b/xen/arch/arm/include/asm/arm32/bitops.h
index d0309d47c1..5552d4e945 100644
--- a/xen/arch/arm/include/asm/arm32/bitops.h
+++ b/xen/arch/arm/include/asm/arm32/bitops.h
@@ -1,7 +1,7 @@
 #ifndef _ARM_ARM32_BITOPS_H
 #define _ARM_ARM32_BITOPS_H
 
-#define flsl fls
+#define arch_flsl fls
 
 /*
  * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
diff --git a/xen/arch/arm/include/asm/arm64/bitops.h 
b/xen/arch/arm/include/asm/arm64/bitops.h
index 0efde29068..5f5d97faa0 100644
--- a/xen/arch/arm/include/asm/arm64/bitops.h
+++ b/xen/arch/arm/include/asm/arm64/bitops.h
@@ -22,17 +22,15 @@ static /*__*/always_inline unsigned long __ffs(unsigned 
long word)
  */
 #define ffz(x)  __ffs(~(x))
 
-static inline int flsl(unsigned long x)
+static inline int arch_flsl(unsigned long x)
 {
 uint64_t ret;
 
-if (__builtin_constant_p(x))
-   return generic_flsl(x);
-
 asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
 
 return BITS_PER_LONG - ret;
 }
+#define arch_flsl arch_flsl
 
 /* Based on linux/include/asm-generic/bitops/find.h */
 
diff --git a/xen/arch/arm/include/asm/bitops.h 
b/xen/arch/arm/include/asm/bitops.h
index 8e16335e76..860d6d4689 100644
--- a/xen/arch/arm/include/asm/bitops.h
+++ b/xen/arch/arm/include/asm/bitops.h
@@ -78,17 +78,14 @@ bool clear_mask16_timeout(uint16_t mask, volatile void *p,
  * the clz instruction for much better code efficiency.
  */
 
-static inline int fls(unsigned int x)
+static inline int arch_fls(unsigned int x)
 {
 int ret;
 
-if (__builtin_constant_p(x))
-   return generic_fls(x);
-
 asm("clz\t%"__OP32"0, %"__OP32"1" : "=r" (ret) : "r" (x));
 return 32 - ret;
 }
-
+#define arch_fls arch_fls
 
 #define arch_ffs(x) ({ unsigned int __t = (x); fls(ISOLATE_LSB(__t)); })
 #define arch_ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); })
diff --git a/xen/arch/ppc/include/asm/bitops.h 
b/xen/arch/ppc/include/asm/bitops.h
index e2b6473c8c..ca308fd62b 100644
--- a/xen/arch/ppc/include/asm/bitops.h
+++ b/xen/arch/ppc/include/asm/bitops.h
@@ -119,9 +119,6 @@ static inline int test_and_set_bit(unsigned int nr, 
volatile void *addr)
 (volatile unsigned int *)addr + BITOP_WORD(nr)) != 0;
 }
 
-#define flsl(x) generic_flsl(x)
-#define fls(x) generic_fls(x)
-
 /* Based on linux/include/asm-generic/bitops/ffz.h */
 /*
  * ffz - find first zero in word.
diff --git a/xen/arch/x86/include/asm/bitops.h 
b/xen/arch/x86/include/asm/bitops.h
index 2b2d9219ef..5d5b9445c5 100644
--- a/xen/arch/x86/include/asm/bitops.h
+++ b/xen/arch/x86/include/asm/bitops.h
@@ -430,7 +430,7 @@ static always_inline unsigned int arch_ffsl(unsigned long x)
  *
  * This is defined the same way as ffs.
  */
-static inline int flsl(unsigned long x)
+static always_inline int arch_flsl(unsigned long x)
 {
 long r;
 
@@ -440,8 +440,9 @@ static inline int flsl(unsigned long x)
   "1:" : "=r" (r) : "rm" (x));
 return (int)r+1;
 }
+#define arch_flsl arch_flsl
 
-static inline int fls(unsigned int x)
+static always_inline int arch_fls(unsigned int x)
 {
 int r;
 
@@ -451,6 +452,7 @@ static inline int fls(unsigned int x)
   "1:" : "=r" (r) : "rm" (x));
 return r + 1;
 }
+#define arch_fls arch_fls
 
 /**
  * hweightN - returns the hamming weight of a N-bit word
diff --git a/xen/common/bitops.c