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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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.

Jan



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