Re: [PATCH] xen/mm: Remove loops in get_order_from_{pages,bytes}()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/10/2026 2:15 pm, Jan Beulich wrote:
> On 28.10.2026 13:41, Andrew Cooper wrote:
>> These two functions are just fls() expressions in disguise.  Furthermore,
>> GCC-12 at least won't const-propagate get_order_from_pages() with an input of
>> 3 or above, which covers most of the cases it gets used for.
>>
>> Rewrite them, and provide some self testing.
>>
>> Interesting, unless they are always_inline, GCC has a habit of out-of-lining
>> them even when inlining would have been a win.  I expect this is to do with
>> the fls() functions being superficially more complex.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> CC: Michal Orzel <michal.orzel@xxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>
>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2999078934
>>
>> I think paddr_t is the wrong type to be using in get_order_from_bytes(), and
>> so really is fls64().
> What other parameter type would you suggest? size_t is suitable only to 
> express
> virtual address region extents.

uint64_t directly.

>
>>  I could add fls_g() which would be slightly nicer on
>> builds where paddr_t != uint64_t.
> We have ffs_g(), so adding fls_g() would only seem natural. But see also 
> below.
> Either form:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

>
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -530,26 +530,16 @@ page_list_splice(struct page_list_head *list, struct 
>> page_list_head *head)
>>      list_for_each_entry_safe_reverse(pos, tmp, head, list)
>>  #endif
>>  
>> -static inline unsigned int get_order_from_bytes(paddr_t size)
>> +static always_inline unsigned int get_order_from_bytes(paddr_t size)
>>  {
>> -    unsigned int order;
>> -
>>      size = (size - 1) >> PAGE_SHIFT;
>> -    for ( order = 0; size; order++ )
>> -        size >>= 1;
>>  
>> -    return order;
>> +    return fls64(size);
>>  }
> Along the lines of what you do below, perhaps simply
>
>     return fls_g(PFN_DOWN(size - 1));
>
> (and nothing else in the function body)? And then even
>
>     return flsl(PFN_DOWN(size - 1));
>
> (eliminating the immediate need for fls_g())?

Neither of these are quite correct.  fls64() can't be downgraded to
flsl(), as it would truncate the input on 32bit builds of Xen.

If using fls_g(), you need to modify size absent of type promotion,
which is why I left the expression broken out in that way.  (I did
actually do fls_g() first, then remove it to shrink the patch.)

I think for now I'll go for the patch as-is.  Any change along these
lines want to be separate anyway.

~Andrew