Re: [PATCH v5 1/8] mm: Place unscrubbed pages at the end of pagelist

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/27/2017 01:06 PM, Jan Beulich wrote:
>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 06/22/17 8:55 PM >>>
>> I kept node_need_scrub[] as a global array and not a "per-node". I think splitting
>> it should be part of making heap_lock a per-node lock, together with increasing
>> scrub concurrency by having more than one CPU scrub a node.
> 
> Agreed - I hadn't meant my earlier comment to necessarily affect this series.
> 
>> @@ -798,11 +814,26 @@ static struct page_info *alloc_heap_pages(
>>       return NULL;
>>   
>>    found:
>> +
>> +    if ( pg->u.free.first_dirty != INVALID_DIRTY_IDX )
>> +        first_dirty_pg = pg + pg->u.free.first_dirty;
>> +
>>       /* We may have to halve the chunk a number of times. */
>>       while ( j != order )
>>       {
>> -        PFN_ORDER(pg) = --j;
>> -        page_list_add_tail(pg, &heap(node, zone, j));
>> +        unsigned int first_dirty;
>> +
>> +        if ( first_dirty_pg && ((pg + (1 << j)) > first_dirty_pg) )
> 
> Despite the various examples of doing it this way, please at least use 1u.
> 
>> +        {
>> +            if ( pg < first_dirty_pg )
>> +                first_dirty = (first_dirty_pg - pg) / sizeof(*pg);
> 
> Pointer subtraction already includes the involved division. 


Yes, this was a mistake.

> Otoh I wonder
> if you couldn't get away without pointer comparison/subtraction here
> altogether.


Without comparison I can only assume that first_dirty is zero (i.e. the 
whole buddy is potentially dirty). Is there something else I could do?


> 
>> @@ -849,13 +880,22 @@ static int reserve_offlined_page(struct page_info *head)
>>   {
>>       unsigned int node = phys_to_nid(page_to_maddr(head));
>>       int zone = page_to_zone(head), i, head_order = PFN_ORDER(head), count = 0;
>> -    struct page_info *cur_head;
>> +    struct page_info *cur_head, *first_dirty_pg = NULL;
>>       int cur_order;
>>   
>>       ASSERT(spin_is_locked(&heap_lock));
>>   
>>       cur_head = head;
>>   
>> +    /*
>> +     * We may break the buddy so let's mark the head as clean. Then, when
>> +     * merging chunks back into the heap, we will see whether the chunk has
>> +     * unscrubbed pages and set its first_dirty properly.
>> +     */
>> +    if (head->u.free.first_dirty != INVALID_DIRTY_IDX)
> 
> Coding style.
> 
>> @@ -892,8 +934,25 @@ static int reserve_offlined_page(struct page_info *head)
>>               {
>>               merge:
>>                   /* We don't consider merging outside the head_order. */
>> -                page_list_add_tail(cur_head, &heap(node, zone, cur_order));
>> -                PFN_ORDER(cur_head) = cur_order;
>> +
>> +                /* See if any of the pages indeed need scrubbing. */
>> +                if ( first_dirty_pg && (cur_head + (1 << cur_order) > first_dirty_pg) )
>> +                {
>> +                    if ( cur_head < first_dirty_pg )
>> +                        i = (first_dirty_pg - cur_head) / sizeof(*cur_head);

I assume the same comment as above applies here.

>> +                    else
>> +                        i = 0;
>> +
>> +                    for ( ; i < (1 << cur_order); i++ )
>> +                        if ( test_bit(_PGC_need_scrub,
>> +                                      &cur_head[i].count_info) )
>> +                        {
>> +                            first_dirty = i;
>> +                            break;
>> +                        }
> 
> Perhaps worth having ASSERT(first_dirty != INVALID_DIRTY_IDX) here? Or are
> there cases where ->u.free.first_dirty of a page may be wrong?


When we merge in free_heap_pages we don't clear first_dirty of the 
successor buddy (at some point I did have this done but you questioned 
whether it was needed and I dropped it).


> 
>> @@ -977,35 +1090,53 @@ static void free_heap_pages(
>>   
>>           if ( (page_to_mfn(pg) & mask) )
>>           {
>> +            struct page_info *predecessor = pg - mask;
>> +
>>               /* Merge with predecessor block? */
>> -            if ( !mfn_valid(_mfn(page_to_mfn(pg-mask))) ||
>> -                 !page_state_is(pg-mask, free) ||
>> -                 (PFN_ORDER(pg-mask) != order) ||
>> -                 (phys_to_nid(page_to_maddr(pg-mask)) != node) )
>> +            if ( !mfn_valid(_mfn(page_to_mfn(predecessor))) ||
>> +                 !page_state_is(predecessor, free) ||
>> +                 (PFN_ORDER(predecessor) != order) ||
>> +                 (phys_to_nid(page_to_maddr(predecessor)) != node) )
>>                   break;
>> -            pg -= mask;
>> -            page_list_del(pg, &heap(node, zone, order));
>> +
>> +            page_list_del(predecessor, &heap(node, zone, order));
>> +
>> +            if ( predecessor->u.free.first_dirty != INVALID_DIRTY_IDX )
>> +                need_scrub = true;
> 
> I'm afraid I continue to be confused by this: Why does need_scrub depend on
> the state of pages not being the subject of the current free operation? I
> realize that at this point in the series the path can't be taken yet, but
> won't later patches need to rip it out or change it anyway, in which case it
> would be better to introduce the then correct check (if any) only there?


Right, at this point we indeed will never have the 'if' evaluate to true 
since heap is always clean. And when we switch to scrubbing from idle 
need_scrub is never looked at.

I suspect this check will not be needed in the intermediate patches neither.

> 
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -88,7 +88,15 @@ struct page_info
>>           /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>>           struct {
>>               /* Do TLBs need flushing for safety before next page use? */
>> -            bool_t need_tlbflush;
>> +            unsigned long need_tlbflush:1;
>> +
>> +            /*
>> +             * Index of the first *possibly* unscrubbed page in the buddy.
>> +             * One more than maximum possible order (MAX_ORDER+1) to
> 
> Why +1 here and hence ...

Don't we have MAX_ORDER+1 orders?

> 
>> +             * accommodate INVALID_DIRTY_IDX.
>> +             */
>> +#define INVALID_DIRTY_IDX (-1UL & (((1UL<<MAX_ORDER) + 2) - 1))
>> +            unsigned long first_dirty:MAX_ORDER + 2;
> 
> ... why +2 instead of +1? And isn't the expression INVALID_DIRTY_IDX wrongly
> parenthesized (apart from lacking blanks around the shift operator)? I'd
> expect you want a value with MAX_ORDER+1 set bits, i.e.
> (1UL << (MAX_ORDER + 1)) - 1. ANDing with -1UL seems quite pointless too.

Yes to parentheses and AND. Should be (1UL << (MAX_ORDER + 2)) - 1

-boris

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