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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 04/04/2017 07:16 AM, Jan Beulich wrote:
>>>> 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.

Wei asked for that too but it wasn't clear to me what the hypervisor
preference is. We currently have much higher use of bool_t than bool
(1015 vs 321, according to cscope). page_alloc.c uses only the former.

>
>> +{
>> +    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.

merge_free_chunks()/can_merge_free() ?

>
>> +         (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.

See patches 2 and 7, where more checks are added. While those two might
be merged (they both deal with scrubbing) , combining all 3 of them into
a single statement would make it a bit difficult to read IMO.

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

All users of merge_chunks() want to place the new chunk back into the
heap and I don't think I see why anyone would want to just get the new
buddy without putting it there.

-boris

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