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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.17 at 18:28, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 03/27/2017 12:03 PM, Jan Beulich wrote:
>>>>> On 27.03.17 at 17:16, <wei.liu2@xxxxxxxxxx> wrote:
>>> On Fri, Mar 24, 2017 at 01:04:56PM -0400, Boris Ostrovsky wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -924,11 +924,61 @@ 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.
>>>
>>>> +                        unsigned int order)
>>>> +{
>>>> +    if ( !mfn_valid(_mfn(page_to_mfn(buddy))) ||
>>>> +         !page_state_is(buddy, free) ||
>>>> +         (PFN_ORDER(buddy) != order) ||
>>>> +         (phys_to_nid(page_to_maddr(buddy)) != node) )
>>>> +        return 0;
>>>> +
>>>> +    return 1;
>>> True and false.
>> Actually there's no point in having two return statements here in
>> the first place the value of the expression (suitably inverted) can
>> be the operand of return.
> 
> Further in the series this routine is expanded with more checks and I
> kept those checks separate since I felt they make it more readable.
> 
> I can certainly merge them all together if people think that it's better
> to have a single return expression.

Well, that depends on how those additions are being made: If the
if() above gets expanded, then the single return statement could
be expanded as well. If, however, you add further return-s (for
clarity, I'd assume), then keeping it the way above (with true/false
used as requested by Wei) would be fine with me.

Jan


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