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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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