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

[Jan Beulich <jbeulich@xxxxxxxx>]

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