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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>  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>

> --- 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())?

Jan

> -static inline unsigned int get_order_from_pages(unsigned long nr_pages)
> +static always_inline unsigned int get_order_from_pages(unsigned long 
> nr_pages)
>  {
> -    unsigned int order;
> -
> -    nr_pages--;
> -    for ( order = 0; nr_pages; order++ )
> -        nr_pages >>= 1;
> -
> -    return order;
> +    return flsl(nr_pages - 1);
>  }
>  
>  #ifndef arch_free_heap_page