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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 08/02/2017 05:24 AM, Jan Beulich wrote:
>>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 07/31/17 6:03 PM >>>
> 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 >>>
>>>>>> --- 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?
> Why 0, 1, 2, and 3? 

Of course, it's 0 and 1 only. MAX_ORDER+1 completely threw me off.

-boris

> When MAX_ORDER is 1, we only have a single bit, i.e.
> valid values 0 and 1 (plus one more for the invalid indicator), i.e. need 2 
> bits
> for representation of all used values.
>
> Jan
>


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