Re: [Xen-devel] [PATCH v5 3/7] x86/hap: improve hypervisor assisted guest TLB flush

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2020 17:57, Roger Pau Monné wrote:
> On Fri, Feb 28, 2020 at 05:44:57PM +0100, Jan Beulich wrote:
>> On 28.02.2020 17:31, Roger Pau Monné wrote:
>>> On Fri, Feb 28, 2020 at 02:58:42PM +0100, Jan Beulich wrote:
>>>> On 19.02.2020 18:43, Roger Pau Monne wrote:
>>>>> @@ -705,20 +701,22 @@ bool hap_flush_tlb(bool (*flush_vcpu)(void *ctxt, 
>>>>> struct vcpu *v),
>>>>>          if ( !flush_vcpu(ctxt, v) )
>>>>>              continue;
>>>>>  
>>>>> -        paging_update_cr3(v, false);
>>>>> +        hvm_asid_flush_vcpu(v);
>>>>>  
>>>>>          cpu = read_atomic(&v->dirty_cpu);
>>>>> -        if ( is_vcpu_dirty_cpu(cpu) )
>>>>> +        if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) )
>>>>>              __cpumask_set_cpu(cpu, mask);
>>>>>      }
>>>>>  
>>>>> -    /* Flush TLBs on all CPUs with dirty vcpu state. */
>>>>> -    flush_tlb_mask(mask);
>>>>> -
>>>>> -    /* Done. */
>>>>> -    for_each_vcpu ( d, v )
>>>>> -        if ( v != current && flush_vcpu(ctxt, v) )
>>>>> -            vcpu_unpause(v);
>>>>> +    /*
>>>>> +     * Trigger a vmexit on all pCPUs with dirty vCPU state in order to 
>>>>> force an
>>>>> +     * ASID/VPID change and hence accomplish a guest TLB flush. Note 
>>>>> that vCPUs
>>>>> +     * not currently running will already be flushed when scheduled 
>>>>> because of
>>>>> +     * the ASID tickle done in the loop above.
>>>>> +     */
>>>>> +    on_selected_cpus(mask, handle_flush, mask, 0);
>>>>> +    while ( !cpumask_empty(mask) )
>>>>> +        cpu_relax();
>>>>
>>>> Why do you pass 0 as last argument? If you passed 1, you wouldn't
>>>> need the while() here, handle_flush() could be empty, and you'd
>>>> save a perhaps large amount of CPUs to all try to modify two
>>>> cache lines (the one used by on_selected_cpus() itself and the
>>>> one here) instead of just one. Yes, latency from the last CPU
>>>> clearing its bit to you being able to exit from here may be a
>>>> little larger this way, but overall latency may shrink with the
>>>> cut by half amount of atomic ops issued to the bus.
>>>
>>> In fact I think passing 0 as the last argument is fine, and
>>> handle_flush can be empty in that case anyway. We don't really care
>>> whether on_selected_cpus returns before all CPUs have executed the
>>> dummy function, as long as all of them have taken a vmexit. Using 0
>>> already guarantees that AFAICT.
>>
>> Isn't it that the caller ants to be guaranteed that the flush
>> has actually occurred (or at least that no further insns can
>> be executed prior to the flush happening, i.e. at least the VM
>> exit having occurred)?
> 
> Yes, but that would happen with 0 as the last argument already, see
> the code in smp_call_function_interrupt:
> 
>     if ( call_data.wait )
>     {
>         (*func)(info);
>         smp_mb();
>         cpumask_clear_cpu(cpu, &call_data.selected);
>     }
>     else
>     {
>         smp_mb();
>         cpumask_clear_cpu(cpu, &call_data.selected);
>         (*func)(info);
>     }
> 
> The only difference is whether on_selected_cpus can return before all
> the functions have been executed on all CPUs, or whether all CPUs have
> taken a vmexit. The later is fine for our use-case.

Oh, yes - right you are.

Jan

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