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

[Boris Ostrovsky]

On 03/28/2017 03:44 PM, Wei Liu wrote:
> On Tue, Mar 28, 2017 at 03:41:10PM -0400, Boris Ostrovsky wrote:
>> On 03/28/2017 03:20 PM, Wei Liu wrote:
>>> On Fri, Mar 24, 2017 at 01:04:56PM -0400, Boris Ostrovsky wrote:
 This is needed for subsequent changes to memory scrubbing. No
 logic change, only code re-factoring.
>>> Actually there is a slight change in logic: pg and order could be
>>> updated in the original merge code. With this patch you still pass the
>>> original pg to reserve_offlined_page if tainted is true. I don't think
>>> this matters in terms of correctness, but it is worth pointing out.
>> Actually, for pg it does matter since reserve_offlined_page() is passed
>> buddy head. So I should make merge_chunks() return new head.
>>
> OK. I think the downside is reserve_offlined_page doesn't merge as many
> pages as it should have, so it is still correct, just the result is
> suboptimal, but I could be wrong -- it's a bit late here. :-)

You are wrong that my patch was right. ;-)

I was passing old head which, if it was merged back in merge_chunks(),
would have lost its PGC_need_scrub bit. And so when
reserve_offlined_page() tries to re-merge buddy's pieces all need_scrub
info would be lost.

Again, this would have been OK with previous version of the series where
every page in the buddy was marked with PGC_need_scrub. But this is no
longer the case.

-boris


>
> I think returning the updated pg is probably best.
>
> Wei.
>
>> ('order' is not used after merge_chunks(), so that's OK).
>>
>> -boris


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

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

[Wei Liu]

On Tue, Mar 28, 2017 at 03:41:10PM -0400, Boris Ostrovsky wrote:
> On 03/28/2017 03:20 PM, Wei Liu wrote:
> > On Fri, Mar 24, 2017 at 01:04:56PM -0400, Boris Ostrovsky wrote:
> >> This is needed for subsequent changes to memory scrubbing. No
> >> logic change, only code re-factoring.
> > Actually there is a slight change in logic: pg and order could be
> > updated in the original merge code. With this patch you still pass the
> > original pg to reserve_offlined_page if tainted is true. I don't think
> > this matters in terms of correctness, but it is worth pointing out.
> 
> Actually, for pg it does matter since reserve_offlined_page() is passed
> buddy head. So I should make merge_chunks() return new head.
> 

OK. I think the downside is reserve_offlined_page doesn't merge as many
pages as it should have, so it is still correct, just the result is
suboptimal, but I could be wrong -- it's a bit late here. :-)

I think returning the updated pg is probably best.

Wei.

> ('order' is not used after merge_chunks(), so that's OK).
> 
> -boris

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

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

[Boris Ostrovsky]

On 03/28/2017 03:20 PM, Wei Liu wrote:
> On Fri, Mar 24, 2017 at 01:04:56PM -0400, Boris Ostrovsky wrote:
>> This is needed for subsequent changes to memory scrubbing. No
>> logic change, only code re-factoring.
> Actually there is a slight change in logic: pg and order could be
> updated in the original merge code. With this patch you still pass the
> original pg to reserve_offlined_page if tainted is true. I don't think
> this matters in terms of correctness, but it is worth pointing out.

Actually, for pg it does matter since reserve_offlined_page() is passed
buddy head. So I should make merge_chunks() return new head.

('order' is not used after merge_chunks(), so that's OK).

-boris

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

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

[Wei Liu]

On Fri, Mar 24, 2017 at 01:04:56PM -0400, Boris Ostrovsky wrote:
> This is needed for subsequent changes to memory scrubbing. No
> logic change, only code re-factoring.

Actually there is a slight change in logic: pg and order could be
updated in the original merge code. With this patch you still pass the
original pg to reserve_offlined_page if tainted is true. I don't think
this matters in terms of correctness, but it is worth pointing out.

Wei.

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

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

[Jan Beulich]

>>> On 27.03.17 at 18:28,  wrote:
> On 03/27/2017 12:03 PM, Jan Beulich wrote:
> On 27.03.17 at 17:16,  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

On 03/27/2017 12:03 PM, Jan Beulich wrote:
 On 27.03.17 at 17:16,  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.

-boris


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

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

[Jan Beulich]

>>> On 27.03.17 at 17:16,  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.

Jan


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

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

[Wei Liu]

On Fri, Mar 24, 2017 at 01:04:56PM -0400, Boris Ostrovsky wrote:
> This is needed for subsequent changes to memory scrubbing. No
> logic change, only code re-factoring.
> 
> Based on earlier patch by Bob Liu.
> 
> Signed-off-by: Boris Ostrovsky 
> ---
>  xen/common/page_alloc.c |   85 --
>  1 files changed, 52 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 42c20cb..7931903 100644
> --- 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.

Other than those:

Reviewed-by: Wei Liu 

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