Re: [PATCH v7 2/9] mm: Place unscrubbed pages at the end of pagelist

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

Hi Jan,

On 15/08/17 15:51, Jan Beulich wrote:
>>>> On 15.08.17 at 16:41, <boris.ostrovsky@oracle.com> wrote:
>> On 08/15/2017 04:18 AM, Jan Beulich wrote:
>>>>>> On 14.08.17 at 16:29, <boris.ostrovsky@oracle.com> wrote:
>>>> On 08/14/2017 06:37 AM, Jan Beulich wrote:
>>>>>>>> On 08.08.17 at 23:45, <boris.ostrovsky@oracle.com> wrote:
>>>>>> --- 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;
>>>>>> +            bool need_tlbflush:1;
>>>>>> +
>>>>>> +            /*
>>>>>> +             * Index of the first *possibly* unscrubbed page in the buddy.
>>>>>> +             * One more bit 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;
>>>>>>          } free;
>>>>> I think generated code will be better with the two fields swapped:
>>>>> That way reading first_dirty won't involve a shift, and accessing a
>>>>> single bit doesn't require shifts at all on many architectures.
>>>> Ok, I will then keep need_tlbflush as the last field so the final struct
>>>> (as defined in patch 7) will look like
>>>>
>>>> struct {
>>>>         unsigned long first_dirty:MAX_ORDER + 1;
>>>>         unsigned long scrub_state:2;
>>>>         bool need_tlbflush:1;
>>>> };
>>> Hmm, actually - why do you need bitfields on the x86 side at all?
>>> They're needed for 32-bit architectures only, 64-bit ones ought
>>> to be fine with
>>>
>>> struct {
>>>         unsigned int first_dirty;
>>>         bool need_tlbflush;
>>>         uint8_t scrub_state;
>>> };
>>
>> IIRC it was exactly because of ARM32 and at some point you suggested to
>> switch both x86 and ARM to bitfields.
>
> I don't recall for sure whether I had asked for the change to be done
> uniformly; it was certainly ARM32 that triggered me notice the
> structure size change in your original version.
>
>>> (plus a suitable BUILD_BUG_ON() to make sure first_dirty has
>>> at least MAX_ORDER + 1 bits). The ARM maintainers will know
>>> whether they would want to also differentiate ARM32 and
>>> ARM64 here.
>>
>> Isn't using bitfields the only possibility for 32-bit? We can't fit
>> first_dirty into 2 bytes.
>
> Yes, hence the question whether to stay with bitfields uniformly
> or make ARM64 follow x86, but ARM32 keep using bitfields.

I would prefer to avoid differentiation between Arm32 and Arm64.

Cheers,

-- 
Julien Grall

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