On 07/10/2018 11:15 AM, Jan Beulich wrote:
>>>> On 10.07.18 at 16:49, <sergey.dya...@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.dya...@citrix.com>
>> ---
>> CC: Andrew Cooper <andrew.coop...@citrix.com>
>> CC: George Dunlap <george.dun...@eu.citrix.com>
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Julien Grall <julien.gr...@arm.com>
>> CC: Wei Liu <wei.l...@citrix.com>
>> CC: Boris Ostrovsky <boris.ostrov...@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 <jbeul...@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
