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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.04.17 at 15:48, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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.

That's because in this release cycle we've only started the transition.

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

Fine with me.

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

Okay, likely makes sense (didn't get that far yet).

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

I understand that, yet the name of the function says otherwise.
At the very least I'd expect a comment right ahead of the function
to state this. Or maybe, as a slight variation of the new name you
suggest above, free_merged_chunks() (merge_chunk_sand_free()
or some such would look ugly to me)?

Jan


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