Re: [Xen-devel] [PATCH v2 1/9] mm: Separate free page chunk merging into its own routine

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.17 at 18:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -924,11 +924,64 @@ static int reserve_offlined_page(struct page_info *head)
>      return count;
>  }
>  
> +static bool_t can_merge(struct page_info *buddy, unsigned int node,

Plain bool please, and const for the pointer.

> +{
> +    if ( !mfn_valid(_mfn(page_to_mfn(buddy))) ||
> +         !page_state_is(buddy, free) ||

As a helper of freeing, this is fine, but then the name of both
functions is too generic: Fundamentally certain other page types
might be mergeable too.

> +         (PFN_ORDER(buddy) != order) ||
> +         (phys_to_nid(page_to_maddr(buddy)) != node) )
> +        return false;
> +
> +    return true;

Is there any reason not to make this a single return statement? I
don't think it would be any worse to read.

> +static struct page_info *merge_chunks(struct page_info *pg, unsigned int 
> node,
> +                                      unsigned int zone, unsigned int order)
> +{
> +    ASSERT(spin_is_locked(&heap_lock));
> +
> +    /* Merge chunks as far as possible. */
> +    while ( order < MAX_ORDER )
> +    {
> +        unsigned long mask = 1UL << order;
> +        struct page_info *buddy;
> +
> +        if ( (page_to_mfn(pg) & mask) )
> +        {
> +            /* Merge with predecessor block? */
> +            buddy = pg - mask;
> +            if ( !can_merge(buddy, node, order) )
> +                break;
> +
> +            pg = buddy;
> +            page_list_del(pg, &heap(node, zone, order));
> +        }
> +        else
> +        {
> +            /* Merge with successor block? */
> +            buddy = pg + mask;
> +            if ( !can_merge(buddy, node, order) )
> +                break;
> +
> +            page_list_del(buddy, &heap(node, zone, order));

This and its companion in the if() branch are now the same (taking
into account the assignment ahead of the former) - please move
them past the if/else.

> +        }
> +
> +        order++;
> +    }
> +
> +    PFN_ORDER(pg) = order;
> +    page_list_add_tail(pg, &heap(node, zone, order));

I don't think this strictly is part of the merge anymore, so judging
by the name of the function this last line would rather belong into
the caller.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel