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

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-05-15 at 11:09 +0200, Jan Beulich wrote:
> On 06.05.2024 12:15, Oleksii Kurochko wrote:
> > Changes in V9:
> >  - update return type of fls and flsl() to unsigned int to be
> > aligned with other
> >    bit ops.
> 
> But this then needs carrying through to ...
> 
> > --- 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)
> 
> ... e.g. here. You don't want to introduce signed/unsigned
> mismatches.
Do you mean that generic flsl() uses 'unsigned int' as a return type,
but arch_flsl continue to use 'int'?

> 
> Also why do you keep "inline" here, while ...
> 
> > --- a/xen/arch/x86/include/asm/bitops.h
> > +++ b/xen/arch/x86/include/asm/bitops.h
> > @@ -425,7 +425,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)
> 
> ... you switch to always_inline here?
Because Adnrew's patch with bitops.h for x86 changes to always_inline,
so to be consistent, at least, for architecture.

~ Oleksii

> 
> (replying out of order)
> 
> > --- 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
> 
> It's the Arm maintainers to ultimately judge, but I'd be inclined to
> suggest
> 
> #define arch_flsl arch_fls
> 
> instead. That's not only behaviorally closer to what was there
> before, but
> also reduces (a tiny bit) the amount of work the compiler needs to
> carry out.
> 
> Jan