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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 07/31/2017 10:45 AM, Jan Beulich wrote:
>>>> 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.

I'll try to see how it looks.

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

Oh, I see what you were asking --- ASSERT() *after* the loop, to make
sure we indeed found the first dirty page. Yes, I will add it.

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


Sorry, I still don't get it.

Say, MAX_ORDER is 1. Since this implies that indexes 0, 1, 2 and 3 are
all valid (because we can have up to 2^(MAX_ORDER+1) pages), don't we
need 3 bits to indicate an invalid index?

-boris

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