[PATCH] mm/page_alloc: correct first_dirty calculations during block merging

[PATCH] mm/page_alloc: correct first_dirty calculations during block merging

[Sergey Dyasli]

Currently it's possible to hit an assertion in alloc_heap_pages():

Assertion 'first_dirty != INVALID_DIRTY_IDX || !(pg[i].count_info & PGC_need_scrub)' failed at page_alloc.c:988

This can happen because a piece of logic to calculate first_dirty
during block merging in free_heap_pages() is missing for the following
scenario:

1. Current block's first_dirty equals to INVALID_DIRTY_IDX
2. Successor block is free but its first_dirty != INVALID_DIRTY_IDX
3. The successor is merged into current block
4. Current block's first_dirty still equals to INVALID_DIRTY_IDX

This will trigger the assertion during allocation of such block in
alloc_heap_pages() because there will be pages with PGC_need_scrub
bit set despite the claim of first_dirty that the block is scrubbed.

Add the missing piece of logic and slightly update the comment for
the predecessor case to better capture the code's intent.

Fixes 1a37f33ea613 ("mm: Place unscrubbed pages at the end of pagelist")

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/common/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 20ee1e4897..aa911f2dc5 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1426,7 +1426,7 @@ static void free_heap_pages(
 
             page_list_del(predecessor, &heap(node, zone, order));
 
-            /* Keep predecessor's first_dirty if it is already set. */
+            /* Keep block's first_dirty if the predecessor doesn't have one */
             if ( predecessor->u.free.first_dirty == INVALID_DIRTY_IDX &&
                  pg->u.free.first_dirty != INVALID_DIRTY_IDX )
                 predecessor->u.free.first_dirty = (1U << order) +
@@ -1447,6 +1447,12 @@ static void free_heap_pages(
 
             check_and_stop_scrub(successor);
 
+            /* Keep successor's first_dirty if the block doesn't have one */
+            if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX &&
+                 successor->u.free.first_dirty != INVALID_DIRTY_IDX )
+                pg->u.free.first_dirty = (1U << order) +
+                                         successor->u.free.first_dirty;
+
             page_list_del(successor, &heap(node, zone, order));
         }
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: correct first_dirty calculations during block merging

[Jan Beulich]

>>> On 10.07.18 at 16:49, <sergey.dyasli@citrix.com> wrote:
> Currently it's possible to hit an assertion in alloc_heap_pages():
> 
> Assertion 'first_dirty != INVALID_DIRTY_IDX || !(pg[i].count_info & 
> PGC_need_scrub)' failed at page_alloc.c:988
> 
> This can happen because a piece of logic to calculate first_dirty
> during block merging in free_heap_pages() is missing for the following
> scenario:
> 
> 1. Current block's first_dirty equals to INVALID_DIRTY_IDX
> 2. Successor block is free but its first_dirty != INVALID_DIRTY_IDX
> 3. The successor is merged into current block
> 4. Current block's first_dirty still equals to INVALID_DIRTY_IDX
> 
> This will trigger the assertion during allocation of such block in
> alloc_heap_pages() because there will be pages with PGC_need_scrub
> bit set despite the claim of first_dirty that the block is scrubbed.
> 
> Add the missing piece of logic and slightly update the comment for
> the predecessor case to better capture the code's intent.
> 
> Fixes 1a37f33ea613 ("mm: Place unscrubbed pages at the end of pagelist")
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  xen/common/page_alloc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 20ee1e4897..aa911f2dc5 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1426,7 +1426,7 @@ static void free_heap_pages(
>  
>              page_list_del(predecessor, &heap(node, zone, order));
>  
> -            /* Keep predecessor's first_dirty if it is already set. */
> +            /* Keep block's first_dirty if the predecessor doesn't have one */
>              if ( predecessor->u.free.first_dirty == INVALID_DIRTY_IDX &&
>                   pg->u.free.first_dirty != INVALID_DIRTY_IDX )
>                  predecessor->u.free.first_dirty = (1U << order) +

How about "Convert pg's first_dirty if predecessor doesn't already have
one"? "Keep" isn't describing well enough what's being done here imo.
Also, despite both styles being okay, I'd prefer to retain the full stop.

> @@ -1447,6 +1447,12 @@ static void free_heap_pages(
>  
>              check_and_stop_scrub(successor);
>  
> +            /* Keep successor's first_dirty if the block doesn't have one */
> +            if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX &&
> +                 successor->u.free.first_dirty != INVALID_DIRTY_IDX )
> +                pg->u.free.first_dirty = (1U << order) +
> +                                         successor->u.free.first_dirty;

Same then accordingly here (and both doable while committing,
provided you agree) and then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: correct first_dirty calculations during block merging

[Boris Ostrovsky]

On 07/10/2018 11:15 AM, Jan Beulich wrote:
>>>> On 10.07.18 at 16:49, <sergey.dyasli@citrix.com> wrote:
>> Currently it's possible to hit an assertion in alloc_heap_pages():
>>
>> Assertion 'first_dirty != INVALID_DIRTY_IDX || !(pg[i].count_info & 
>> PGC_need_scrub)' failed at page_alloc.c:988
>>
>> This can happen because a piece of logic to calculate first_dirty
>> during block merging in free_heap_pages() is missing for the following
>> scenario:
>>
>> 1. Current block's first_dirty equals to INVALID_DIRTY_IDX
>> 2. Successor block is free but its first_dirty != INVALID_DIRTY_IDX
>> 3. The successor is merged into current block
>> 4. Current block's first_dirty still equals to INVALID_DIRTY_IDX
>>
>> This will trigger the assertion during allocation of such block in
>> alloc_heap_pages() because there will be pages with PGC_need_scrub
>> bit set despite the claim of first_dirty that the block is scrubbed.
>>
>> Add the missing piece of logic and slightly update the comment for
>> the predecessor case to better capture the code's intent.
>>
>> Fixes 1a37f33ea613 ("mm: Place unscrubbed pages at the end of pagelist")
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> ---
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>  xen/common/page_alloc.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 20ee1e4897..aa911f2dc5 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1426,7 +1426,7 @@ static void free_heap_pages(
>>  
>>              page_list_del(predecessor, &heap(node, zone, order));
>>  
>> -            /* Keep predecessor's first_dirty if it is already set. */
>> +            /* Keep block's first_dirty if the predecessor doesn't have one */
>>              if ( predecessor->u.free.first_dirty == INVALID_DIRTY_IDX &&
>>                   pg->u.free.first_dirty != INVALID_DIRTY_IDX )
>>                  predecessor->u.free.first_dirty = (1U << order) +
> How about "Convert pg's first_dirty if predecessor doesn't already have
> one"? "Keep" isn't describing well enough what's being done here imo.

"Keep" was used here for the (not provided) "else" clause. But I can see
how it can be confusing.

"Update predecessor's first_dirty if necessary"? Or maybe even drop it.


> Also, despite both styles being okay, I'd prefer to retain the full stop.
>
>> @@ -1447,6 +1447,12 @@ static void free_heap_pages(
>>  
>>              check_and_stop_scrub(successor);
>>  
>> +            /* Keep successor's first_dirty if the block doesn't have one */
>> +            if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX &&
>> +                 successor->u.free.first_dirty != INVALID_DIRTY_IDX )
>> +                pg->u.free.first_dirty = (1U << order) +
>> +                                         successor->u.free.first_dirty;
> Same then accordingly here (and both doable while committing,
> provided you agree) and then
> Reviewed-by: Jan Beulich <jbeulich@suse.com>


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: correct first_dirty calculations during block merging

[Sergey Dyasli]

On Tue, 2018-07-10 at 11:34 -0400, Boris Ostrovsky wrote:
> On 07/10/2018 11:15 AM, Jan Beulich wrote:
> > > > > On 10.07.18 at 16:49, <sergey.dyasli@citrix.com> wrote:
> > > 
> > > Currently it's possible to hit an assertion in alloc_heap_pages():
> > > 
> > > Assertion 'first_dirty != INVALID_DIRTY_IDX || !(pg[i].count_info & 
> > > PGC_need_scrub)' failed at page_alloc.c:988
> > > 
> > > This can happen because a piece of logic to calculate first_dirty
> > > during block merging in free_heap_pages() is missing for the following
> > > scenario:
> > > 
> > > 1. Current block's first_dirty equals to INVALID_DIRTY_IDX
> > > 2. Successor block is free but its first_dirty != INVALID_DIRTY_IDX
> > > 3. The successor is merged into current block
> > > 4. Current block's first_dirty still equals to INVALID_DIRTY_IDX
> > > 
> > > This will trigger the assertion during allocation of such block in
> > > alloc_heap_pages() because there will be pages with PGC_need_scrub
> > > bit set despite the claim of first_dirty that the block is scrubbed.
> > > 
> > > Add the missing piece of logic and slightly update the comment for
> > > the predecessor case to better capture the code's intent.
> > > 
> > > Fixes 1a37f33ea613 ("mm: Place unscrubbed pages at the end of pagelist")
> > > 
> > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> > > ---
> > > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > > CC: George Dunlap <George.Dunlap@eu.citrix.com>
> > > CC: Jan Beulich <jbeulich@suse.com>
> > > CC: Julien Grall <julien.grall@arm.com>
> > > CC: Wei Liu <wei.liu2@citrix.com>
> > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > ---
> > >  xen/common/page_alloc.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > > index 20ee1e4897..aa911f2dc5 100644
> > > --- a/xen/common/page_alloc.c
> > > +++ b/xen/common/page_alloc.c
> > > @@ -1426,7 +1426,7 @@ static void free_heap_pages(
> > >  
> > >              page_list_del(predecessor, &heap(node, zone, order));
> > >  
> > > -            /* Keep predecessor's first_dirty if it is already set. */
> > > +            /* Keep block's first_dirty if the predecessor doesn't have one */
> > >              if ( predecessor->u.free.first_dirty == INVALID_DIRTY_IDX &&
> > >                   pg->u.free.first_dirty != INVALID_DIRTY_IDX )
> > >                  predecessor->u.free.first_dirty = (1U << order) +
> > 
> > How about "Convert pg's first_dirty if predecessor doesn't already have
> > one"? "Keep" isn't describing well enough what's being done here imo.
> 
> "Keep" was used here for the (not provided) "else" clause. But I can see
> how it can be confusing.
> 
> "Update predecessor's first_dirty if necessary"? Or maybe even drop it.

I'd like to retain the comments. Personally, I like the following
variant because the if statement logic is pretty self-explanatory:

    /* Update predecessor's first_dirty if necessary */
    ...
    /* Update pg's first_dirty if necessary */
    
These changes can be done while committing.

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel