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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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


I am not sure I see how we are mixing the semantics here. <datatype>:1
is really a tightly-packed bool.

Switching to bitfields was, btw, suggested by Jan at some point so if
the two of you agree on how to proceed I can go either way (but by
preference is to keep it as a single-bit bitfield).


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

Oh, I see. Sure, I'll add it.

-boris


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