* [PATCH] x86/hap: be more selective with assisted TLB flush
@ 2020-04-29 17:36 Roger Pau Monne
2020-04-30 7:20 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monne @ 2020-04-29 17:36 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, George Dunlap, Jan Beulich, Roger Pau Monne
When doing an assisted flush on HAP the purpose of the
on_selected_cpus is just to trigger a vmexit on remote CPUs that are
in guest context, and hence just using is_vcpu_dirty_cpu is too lax,
also check that the vCPU is running.
While there also pass NULL as the data parameter of on_selected_cpus,
the dummy handler doesn't consume the data in any way.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/mm/hap/hap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 580d1c2164..0275cdf5c8 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -719,7 +719,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
hvm_asid_flush_vcpu(v);
cpu = read_atomic(&v->dirty_cpu);
- if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) )
+ if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) && v->is_running )
__cpumask_set_cpu(cpu, mask);
}
@@ -729,7 +729,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
* not currently running will already be flushed when scheduled because of
* the ASID tickle done in the loop above.
*/
- on_selected_cpus(mask, dummy_flush, mask, 0);
+ on_selected_cpus(mask, dummy_flush, NULL, 0);
return true;
}
--
2.26.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] x86/hap: be more selective with assisted TLB flush
2020-04-29 17:36 [PATCH] x86/hap: be more selective with assisted TLB flush Roger Pau Monne
@ 2020-04-30 7:20 ` Jan Beulich
2020-04-30 8:28 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-04-30 7:20 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, George Dunlap, Wei Liu, Andrew Cooper
On 29.04.2020 19:36, Roger Pau Monne wrote:
> When doing an assisted flush on HAP the purpose of the
> on_selected_cpus is just to trigger a vmexit on remote CPUs that are
> in guest context, and hence just using is_vcpu_dirty_cpu is too lax,
> also check that the vCPU is running.
Am I right to understand that the change is relevant only to
cover the period of time between ->is_running becoming false
and ->dirty_cpu becoming VCPU_CPU_CLEAN? I.e. ...
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -719,7 +719,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
> hvm_asid_flush_vcpu(v);
>
> cpu = read_atomic(&v->dirty_cpu);
> - if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) )
> + if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) && v->is_running )
... the previous logic would have suitably covered the switch-to
path, but doesn't properly cover the switch-from one, due to our
lazy context switch approach? If so, I agree with the change:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
It might be worth mentioning this detail in the description then,
though.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/hap: be more selective with assisted TLB flush
2020-04-30 7:20 ` Jan Beulich
@ 2020-04-30 8:28 ` Roger Pau Monné
2020-04-30 8:33 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2020-04-30 8:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, George Dunlap, Wei Liu, Andrew Cooper
On Thu, Apr 30, 2020 at 09:20:58AM +0200, Jan Beulich wrote:
> On 29.04.2020 19:36, Roger Pau Monne wrote:
> > When doing an assisted flush on HAP the purpose of the
> > on_selected_cpus is just to trigger a vmexit on remote CPUs that are
> > in guest context, and hence just using is_vcpu_dirty_cpu is too lax,
> > also check that the vCPU is running.
>
> Am I right to understand that the change is relevant only to
> cover the period of time between ->is_running becoming false
> and ->dirty_cpu becoming VCPU_CPU_CLEAN? I.e. ...
>
> > --- a/xen/arch/x86/mm/hap/hap.c
> > +++ b/xen/arch/x86/mm/hap/hap.c
> > @@ -719,7 +719,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
> > hvm_asid_flush_vcpu(v);
> >
> > cpu = read_atomic(&v->dirty_cpu);
> > - if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) )
> > + if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) && v->is_running )
>
> ... the previous logic would have suitably covered the switch-to
> path, but doesn't properly cover the switch-from one, due to our
> lazy context switch approach?
Yes. Also __context_switch is not called from context_switch when
switching to the idle vcpu, and hence dirty_cpu is not cleared.
> If so, I agree with the change:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> It might be worth mentioning this detail in the description then,
> though.
Would you mind adding to the commit message if you agree:
"Due to the lazy context switching done by Xen dirty_cpu won't always be
cleared when the guest vCPU is not running, and hence relying on
is_running allows more fine grained control of whether the vCPU is
actually running."
Thanks, Roger.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/hap: be more selective with assisted TLB flush
2020-04-30 8:28 ` Roger Pau Monné
@ 2020-04-30 8:33 ` Jan Beulich
2020-04-30 16:19 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-04-30 8:33 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, George Dunlap, Wei Liu, Andrew Cooper
On 30.04.2020 10:28, Roger Pau Monné wrote:
> On Thu, Apr 30, 2020 at 09:20:58AM +0200, Jan Beulich wrote:
>> On 29.04.2020 19:36, Roger Pau Monne wrote:
>>> When doing an assisted flush on HAP the purpose of the
>>> on_selected_cpus is just to trigger a vmexit on remote CPUs that are
>>> in guest context, and hence just using is_vcpu_dirty_cpu is too lax,
>>> also check that the vCPU is running.
>>
>> Am I right to understand that the change is relevant only to
>> cover the period of time between ->is_running becoming false
>> and ->dirty_cpu becoming VCPU_CPU_CLEAN? I.e. ...
>>
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -719,7 +719,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
>>> hvm_asid_flush_vcpu(v);
>>>
>>> cpu = read_atomic(&v->dirty_cpu);
>>> - if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) )
>>> + if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) && v->is_running )
>>
>> ... the previous logic would have suitably covered the switch-to
>> path, but doesn't properly cover the switch-from one, due to our
>> lazy context switch approach?
>
> Yes. Also __context_switch is not called from context_switch when
> switching to the idle vcpu, and hence dirty_cpu is not cleared.
>
>> If so, I agree with the change:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> It might be worth mentioning this detail in the description then,
>> though.
>
> Would you mind adding to the commit message if you agree:
>
> "Due to the lazy context switching done by Xen dirty_cpu won't always be
> cleared when the guest vCPU is not running, and hence relying on
> is_running allows more fine grained control of whether the vCPU is
> actually running."
Sure; I'll give it over the weekend though for others to comment, if
so desired.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/hap: be more selective with assisted TLB flush
2020-04-30 8:33 ` Jan Beulich
@ 2020-04-30 16:19 ` Andrew Cooper
2020-04-30 18:09 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2020-04-30 16:19 UTC (permalink / raw)
To: Jan Beulich, Roger Pau Monné; +Cc: xen-devel, George Dunlap, Wei Liu
On 30/04/2020 09:33, Jan Beulich wrote:
> On 30.04.2020 10:28, Roger Pau Monné wrote:
>> On Thu, Apr 30, 2020 at 09:20:58AM +0200, Jan Beulich wrote:
>>> On 29.04.2020 19:36, Roger Pau Monne wrote:
>>>> When doing an assisted flush on HAP the purpose of the
>>>> on_selected_cpus is just to trigger a vmexit on remote CPUs that are
>>>> in guest context, and hence just using is_vcpu_dirty_cpu is too lax,
>>>> also check that the vCPU is running.
>>> Am I right to understand that the change is relevant only to
>>> cover the period of time between ->is_running becoming false
>>> and ->dirty_cpu becoming VCPU_CPU_CLEAN? I.e. ...
>>>
>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>> @@ -719,7 +719,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
>>>> hvm_asid_flush_vcpu(v);
>>>>
>>>> cpu = read_atomic(&v->dirty_cpu);
>>>> - if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) )
>>>> + if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) && v->is_running )
>>> ... the previous logic would have suitably covered the switch-to
>>> path, but doesn't properly cover the switch-from one, due to our
>>> lazy context switch approach?
>> Yes. Also __context_switch is not called from context_switch when
>> switching to the idle vcpu, and hence dirty_cpu is not cleared.
>>
>>> If so, I agree with the change:
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> It might be worth mentioning this detail in the description then,
>>> though.
>> Would you mind adding to the commit message if you agree:
>>
>> "Due to the lazy context switching done by Xen dirty_cpu won't always be
>> cleared when the guest vCPU is not running, and hence relying on
>> is_running allows more fine grained control of whether the vCPU is
>> actually running."
> Sure; I'll give it over the weekend though for others to comment, if
> so desired.
I think it is worth pointing out that this fixes a large perf regression
on Nehalem/Westmere systems, where L1 Shim using the enlightened
hypercall is 8x slower than unenlightened way.
~Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/hap: be more selective with assisted TLB flush
2020-04-30 16:19 ` Andrew Cooper
@ 2020-04-30 18:09 ` Roger Pau Monné
0 siblings, 0 replies; 6+ messages in thread
From: Roger Pau Monné @ 2020-04-30 18:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, George Dunlap, Jan Beulich
On Thu, Apr 30, 2020 at 05:19:19PM +0100, Andrew Cooper wrote:
> On 30/04/2020 09:33, Jan Beulich wrote:
> > On 30.04.2020 10:28, Roger Pau Monné wrote:
> >> On Thu, Apr 30, 2020 at 09:20:58AM +0200, Jan Beulich wrote:
> >>> On 29.04.2020 19:36, Roger Pau Monne wrote:
> >>>> When doing an assisted flush on HAP the purpose of the
> >>>> on_selected_cpus is just to trigger a vmexit on remote CPUs that are
> >>>> in guest context, and hence just using is_vcpu_dirty_cpu is too lax,
> >>>> also check that the vCPU is running.
> >>> Am I right to understand that the change is relevant only to
> >>> cover the period of time between ->is_running becoming false
> >>> and ->dirty_cpu becoming VCPU_CPU_CLEAN? I.e. ...
> >>>
> >>>> --- a/xen/arch/x86/mm/hap/hap.c
> >>>> +++ b/xen/arch/x86/mm/hap/hap.c
> >>>> @@ -719,7 +719,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
> >>>> hvm_asid_flush_vcpu(v);
> >>>>
> >>>> cpu = read_atomic(&v->dirty_cpu);
> >>>> - if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) )
> >>>> + if ( cpu != this_cpu && is_vcpu_dirty_cpu(cpu) && v->is_running )
> >>> ... the previous logic would have suitably covered the switch-to
> >>> path, but doesn't properly cover the switch-from one, due to our
> >>> lazy context switch approach?
> >> Yes. Also __context_switch is not called from context_switch when
> >> switching to the idle vcpu, and hence dirty_cpu is not cleared.
> >>
> >>> If so, I agree with the change:
> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>> It might be worth mentioning this detail in the description then,
> >>> though.
> >> Would you mind adding to the commit message if you agree:
> >>
> >> "Due to the lazy context switching done by Xen dirty_cpu won't always be
> >> cleared when the guest vCPU is not running, and hence relying on
> >> is_running allows more fine grained control of whether the vCPU is
> >> actually running."
> > Sure; I'll give it over the weekend though for others to comment, if
> > so desired.
>
> I think it is worth pointing out that this fixes a large perf regression
> on Nehalem/Westmere systems, where L1 Shim using the enlightened
> hypercall is 8x slower than unenlightened way.
I might as well post the actual numbers I have.
I've measured the time of the non-local branch of flush_area_mask
inside the shim running with 32vCPUs over 100000 executions and
averaged the result on a large Westmere system (80 ways total). The
figures where fetched during the boot of a SLES 11 PV guest. The
results are as follow (less is better):
Non assisted flush with x2APIC: 112406ns
Assisted flush without this patch: 820450ns
Assisted flush with this patch: 8330ns
I can add the figures to the commit message if deemed interesting to
have in the repo. Or the above text can be appended to the commit
message if that's fine.
Roger.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-30 18:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-29 17:36 [PATCH] x86/hap: be more selective with assisted TLB flush Roger Pau Monne
2020-04-30 7:20 ` Jan Beulich
2020-04-30 8:28 ` Roger Pau Monné
2020-04-30 8:33 ` Jan Beulich
2020-04-30 16:19 ` Andrew Cooper
2020-04-30 18:09 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.