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

[Julien Grall <julien.grall@arm.com>]

Hi,

On 07/08/17 15:46, Boris Ostrovsky wrote:
> On 08/07/2017 06:45 AM, Julien Grall wrote:
>> Hi Boris,
>>
>> I would have appreciated to be CCed as maintainer of the ARM bits...
>> Please use scripts/get_maintainers.pl in the future.
>
> Ugh, sorry about that. (I did test builds for both ARM64 and ARM32, if
> this make my transgression any less serious ;-))
>
>>
>> On 04/08/17 18:05, Boris Ostrovsky wrote:
>>> . so that it's easy to find pages that need to be scrubbed (those
>>> pages are
>>
>> Pointless .
>>
>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>> index ef84b72..d26b232 100644
>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -44,7 +44,16 @@ 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;
>>
>> You've turned need_tlbflush from bool to unsigned long : 1. But some
>> of the users use true/false or may rely on the boolean property.  So
>> it sounds like to me you want to use bool bitfields here (and in the
>> x86 part).
>
> This is what we have (with this series applied):
>
> root@ovs104> git grep need_tlbflush .
> common/memory.c:    bool need_tlbflush = false;
> common/memory.c:
> accumulate_tlbflush(&need_tlbflush, &page[j],
> common/memory.c:    if ( need_tlbflush )
> common/page_alloc.c:    bool need_tlbflush = false;
> common/page_alloc.c:            accumulate_tlbflush(&need_tlbflush, &pg[i],
> common/page_alloc.c:    if ( need_tlbflush )
> * common/page_alloc.c:        pg[i].u.free.need_tlbflush =
> (page_get_owner(&pg[i]) != NULL);
> common/page_alloc.c:        if ( pg[i].u.free.need_tlbflush )
> include/asm-arm/mm.h:                unsigned long need_tlbflush:1;
> include/asm-x86/mm.h:           unsigned long need_tlbflush:1;
> include/xen/mm.h:static inline void accumulate_tlbflush(bool *need_tlbflush,
> include/xen/mm.h:    if ( page->u.free.need_tlbflush &&
> include/xen/mm.h:         (!*need_tlbflush ||
> include/xen/mm.h:        *need_tlbflush = true;
> root@ovs104>
>
> The only possibly boolean-style use is '*' and event that I think is
> allowed.
>
> Stand-alone need_tlbflush variables above have nothing to do with this
> change.

If you look at it, all of them use bool semantic. Now you mix bool and 
int. We are trying to remove that in the new code, so please don't 
introduce new one.

>
>
>>
>>> +
>>> +            /*
>>> +             * Index of the first *possibly* unscrubbed page in the
>>> buddy.
>>> +             * One more than maximum possible order to accommodate
>>> +             * INVALID_DIRTY_IDX.
>>> +             */
>>> +#define INVALID_DIRTY_IDX ((1UL << (MAX_ORDER + 1)) - 1)
>>> +            unsigned long first_dirty:MAX_ORDER + 1;
>>
>> We need to make sure that this union will not be bigger than unsigned
>> long. Otherwise this will limit lower down the maximum amount of
>> memory we support.
>> So this likely means a BUILD_BUG_ON(....).
>
>
> Are you concerned that (MAX_ORDER+1) will be larger than sizeof(unsigned
> long)? If yes, the compiler should complain anyway, shouldn't it?

I am more concerned about the sizeof of the union u to be larger than 
unsigned long.

first_dirty should not be greater than 63 bits (or 31 bits for 32-bits 
architecture). Otherwise likely the compiler will add a padding between 
need_tlbflush and first_dirty. This would increase the page_info by 4/8 
byte.

The BUILD_BUG_ON(...) would be here to catch such error.

Cheers,

-- 
Julien Grall

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