Re: [PATCH] xen/mm: page_alloc fix duplicated order shift operation in the loop

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2022 21:35, Paran Lee wrote:
> It doesn't seem necessary to do that
> duplicate calculation of order shift 2^@order in the loop.

Once again I'm not convinced. As in the other patch the compiler could
do this transformation via its CSE pass if it sees fit.

Also (applicable as well to the other patch) I think "fix" in the
subject is misleading: There's nothing wrong with the original code.

> In addition, I fixed type of total_avail_pages from long
> to unsigned long. because when total_avail_pages static variable
> substitute in functions of page alloc local variable,
> type of local variables is unsigned long.

You've done more changes, some of which are questionable.

> @@ -922,8 +922,9 @@ static struct page_info *alloc_heap_pages(
>      struct domain *d)
>  {
>      nodeid_t node;
> -    unsigned int i, buddy_order, zone, first_dirty;
> -    unsigned long request = 1UL << order;
> +    unsigned int buddy_order, zone, first_dirty;
> +    unsigned int buddy_request;
> +    unsigned long i, request = 1UL << order;

E.g. it's not clear why these both need to be unsigned long when the
largest chunk which can be allocated in one go is 1GiB (MAX_ORDER).
At least on x86 operations on 32-bit quantities are generally
slightly more efficient than such on 64-bit values. If we wanted to
cater for architectures setting MAX_ORDER to 32 or higher, I think
the type used should become a typedef picking "unsigned int" at least
on x86.

> @@ -975,16 +976,17 @@ static struct page_info *alloc_heap_pages(
>      while ( buddy_order != order )
>      {
>          buddy_order--;
> +        buddy_request = 1U << buddy_order;

Such a local variable would better have narrowest possible scope.

> @@ -1490,7 +1493,7 @@ static void free_heap_pages(
>              /* Update predecessor's first_dirty if necessary. */
>              if ( predecessor->u.free.first_dirty == INVALID_DIRTY_IDX &&
>                   pg->u.free.first_dirty != INVALID_DIRTY_IDX )
> -                predecessor->u.free.first_dirty = (1U << order) +
> +                predecessor->u.free.first_dirty = mask +
>                                                    pg->u.free.first_dirty;
>  
>              pg = predecessor;
> @@ -1511,7 +1514,7 @@ static void free_heap_pages(
>              /* Update pg's first_dirty if necessary. */
>              if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX &&
>                   successor->u.free.first_dirty != INVALID_DIRTY_IDX )
> -                pg->u.free.first_dirty = (1U << order) +
> +                pg->u.free.first_dirty = mask +
>                                           successor->u.free.first_dirty;

This re-use of an existing local variable looks reasonable to me.
It would be nice though if the variable's scope was shrunk and its
type was also adjusted to unsigned int.

Jan