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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 07/23/17 4:01 AM >>>
>On 06/27/2017 01:06 PM, Jan Beulich wrote:
>>>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 06/22/17 8:55 PM >>>
>>> +        {
>>> +            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?

I was thinking of tracking indexes instead of pointers. But maybe that
would more hamper readability of the overall result than help it.
 
>>> @@ -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.

Of course. I usually avoid repeating the same comment, except maybe
when reviewing patches of first time contributors.

>>> +                    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).

Hmm, this indeed answers my question, but doesn't help (me) understanding
whether the suggested ASSERT() could be wrong.

>>> --- 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?

So here there might be a simple misunderstanding: I understand the
parenthesized MAX_ORDER+1 to represent "maximum possible
order", i.e. excluding the "one more than", not the least because of
the ...

>> +             * accommodate INVALID_DIRTY_IDX.
>> +             */
>> +#define INVALID_DIRTY_IDX (-1UL & (((1UL<<MAX_ORDER) + 2) - 1))
>> +            unsigned long first_dirty:MAX_ORDER + 2;

+2 here.

>> ... 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

I.e. I would still expect it to be (1UL << (MAX_ORDER + 1)) - 1
here.

Jan


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