[Xen-devel] Ping: Re: [PATCH] x86: correct vCPU dirty CPU handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.04.18 at 12:52, <JBeulich@xxxxxxxx> wrote:
>>>> On 26.04.18 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 26/04/18 10:41, Jan Beulich wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>>>               unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
>>>               (l1e_owner == pg_owner) )
>>>          {
>>> +            cpumask_t *mask = this_cpu(scratch_cpumask);
>>> +
>>> +            cpumask_clear(mask);
>>> +
>>>              for_each_vcpu ( pg_owner, v )
>>>              {
>>> -                if ( pv_destroy_ldt(v) )
>>> -                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
>>> +                unsigned int cpu;
>>> +
>>> +                if ( !pv_destroy_ldt(v) )
>>> +                    continue;
>>> +                cpu = read_atomic(&v->dirty_cpu);
>>> +                if ( is_vcpu_dirty_cpu(cpu) )
>>> +                    __cpumask_set_cpu(cpu, mask);
>>>              }
>>> +
>>> +            if ( !cpumask_empty(mask) )
>>> +                flush_tlb_mask(mask);
>> 
>> Thinking about this, what is wrong with:
>> 
>> bool flush;
>> 
>> for_each_vcpu ( pg_owner, v )
>>     if ( pv_destroy_ldt(v) )
>>         flush = true;
>> 
>> if ( flush )
>>    flush_tlb_mask(pg_owner->dirty_cpumask);
>> 
>> This is far less complicated cpumask handling.  As the loop may be long,
>> it avoids flushing pcpus which have subsequently switched away from
>> pg_owner context.  It also avoids all playing with v->dirty_cpu.
> 
> That would look to be correct, but I'm not sure it would be an improvement:
> While it may avoid flushing some CPUs, it may then do extra flushes on
> others (which another vCPU of the domain has been switched to). Plus it
> would flush even those CPUs where pv_destroy_ldt() has returned false,
> as long as the function returned true at least once.

Ping?

> If I was to go that route, I'd at least extend to latching
> pg_owner->dirty_cpumask before the loop into scratch_cpumask, ANDing
> in pg_owner->dirty_cpumask after the loop to restrict to those CPUs which
> may have remained active over the entire time the loop takes. But even
> then I would still be afraid of flushing far more CPUs than actually needed.

I don't think anymore that this would be correct, so please ignore this
part.

Thanks, Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel