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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 08/15/2017 10:52 AM, Julien Grall wrote:
> Hi Jan,
>
> On 15/08/17 15:51, Jan Beulich wrote:
>>>>> On 15.08.17 at 16:41, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 08/15/2017 04:18 AM, Jan Beulich wrote:
>>>>>>> On 14.08.17 at 16:29, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> On 08/14/2017 06:37 AM, Jan Beulich wrote:
>>>>>>>>> On 08.08.17 at 23:45, <boris.ostrovsky@xxxxxxxxxx> 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.

I can switch x86 only back to "normal" types but then we also have this
in patch 7:

static void check_and_stop_scrub(struct page_info *head)
{
    if ( head->u.free.scrub_state == BUDDY_SCRUBBING )
    {
        typeof(head->u.free) pgfree;

        head->u.free.scrub_state = BUDDY_SCRUB_ABORT;
        spin_lock_kick();
        for ( ; ; )
        {
            /* Can't ACCESS_ONCE() a bitfield. */
            pgfree.val = ACCESS_ONCE(head->u.free.val);
            if ( pgfree.scrub_state != BUDDY_SCRUB_ABORT )
                break;
            cpu_relax();
        }
    }
}

I'd rather avoid having '#ifdef <arch>' meaning that

union {
            struct {
                unsigned int first_dirty;
                bool need_tlbflush;
                uint8_t scrub_state;
            };

            unsigned long val;
} free;

is still kept for x86.

-boris

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