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

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

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.


>
>> +
>> +            /*
>> +             * 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?


-boris



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