* [PATCH] KVM: Clear pv eoi pending bit only when it is set
@ 2021-10-19 2:05 Li RongQing
2021-10-19 7:23 ` Vitaly Kuznetsov
0 siblings, 1 reply; 6+ messages in thread
From: Li RongQing @ 2021-10-19 2:05 UTC (permalink / raw)
To: lirongqing, pbonzini, seanjc, vkuznets, wanpengli, jmattson,
joro, tglx, mingo, bp, x86, hpa, kvm
clear pv eoi pending bit only when it is set, to avoid calling
pv_eoi_put_user()
and this can speed pv_eoi_clr_pending about 300 nsec on AMD EPYC
most of the time
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
arch/x86/kvm/lapic.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 76fb009..c434f70 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -694,9 +694,9 @@ static void pv_eoi_set_pending(struct kvm_vcpu *vcpu)
__set_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
}
-static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
+static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu, bool pending)
{
- if (pv_eoi_put_user(vcpu, KVM_PV_EOI_DISABLED) < 0) {
+ if (pending && pv_eoi_put_user(vcpu, KVM_PV_EOI_DISABLED) < 0) {
printk(KERN_WARNING "Can't clear EOI MSR value: 0x%llx\n",
(unsigned long long)vcpu->arch.pv_eoi.msr_val);
return;
@@ -2693,7 +2693,8 @@ static void apic_sync_pv_eoi_from_guest(struct kvm_vcpu *vcpu,
* While this might not be ideal from performance point of view,
* this makes sure pv eoi is only enabled when we know it's safe.
*/
- pv_eoi_clr_pending(vcpu);
+ pv_eoi_clr_pending(vcpu, pending);
+
if (pending)
return;
vector = apic_set_eoi(apic);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: Clear pv eoi pending bit only when it is set
2021-10-19 2:05 [PATCH] KVM: Clear pv eoi pending bit only when it is set Li RongQing
@ 2021-10-19 7:23 ` Vitaly Kuznetsov
2021-10-19 7:29 ` Paolo Bonzini
2021-10-20 3:43 ` Li,Rongqing
0 siblings, 2 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2021-10-19 7:23 UTC (permalink / raw)
To: Li RongQing
Cc: lirongqing, pbonzini, seanjc, wanpengli, jmattson, joro, tglx,
mingo, bp, x86, hpa, kvm
Li RongQing <lirongqing@baidu.com> writes:
> clear pv eoi pending bit only when it is set, to avoid calling
> pv_eoi_put_user()
>
> and this can speed pv_eoi_clr_pending about 300 nsec on AMD EPYC
> most of the time
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> arch/x86/kvm/lapic.c | 7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 76fb009..c434f70 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -694,9 +694,9 @@ static void pv_eoi_set_pending(struct kvm_vcpu *vcpu)
> __set_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> }
>
> -static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
> +static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu, bool pending)
Nitpick (and probably a matter of personal taste): pv_eoi_clr_pending()
has only one user and the change doesn't make its interface much nicer,
I'd suggest we just inline in instead. (we can probably do the same to
pv_eoi_get_pending()/pv_eoi_set_pending() too).
> {
> - if (pv_eoi_put_user(vcpu, KVM_PV_EOI_DISABLED) < 0) {
> + if (pending && pv_eoi_put_user(vcpu, KVM_PV_EOI_DISABLED) < 0) {
> printk(KERN_WARNING "Can't clear EOI MSR value: 0x%llx\n",
> (unsigned long long)vcpu->arch.pv_eoi.msr_val);
> return;
> @@ -2693,7 +2693,8 @@ static void apic_sync_pv_eoi_from_guest(struct kvm_vcpu *vcpu,
> * While this might not be ideal from performance point of view,
> * this makes sure pv eoi is only enabled when we know it's safe.
> */
> - pv_eoi_clr_pending(vcpu);
> + pv_eoi_clr_pending(vcpu, pending);
> +
> if (pending)
> return;
> vector = apic_set_eoi(apic);
Could you probably elaborate a bit (probably by enhancing the comment
above pv_eoi_clr_pending()) why the race we have here (even before the
patch) doesn't matter? As far as I understand it, the guest can change
PV EOI status from a different CPU (it shouldn't do it but it still can)
at any time: e.g. between pv_eoi_get_pending() and pv_eoi_clr_pending()
but also right after we do pv_eoi_clr_pending() so the patch doesn't
really change much in this regard.
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: Clear pv eoi pending bit only when it is set
2021-10-19 7:23 ` Vitaly Kuznetsov
@ 2021-10-19 7:29 ` Paolo Bonzini
2021-10-20 3:36 ` 答复: " Li,Rongqing
2021-10-20 11:15 ` Li,Rongqing
2021-10-20 3:43 ` Li,Rongqing
1 sibling, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-10-19 7:29 UTC (permalink / raw)
To: Vitaly Kuznetsov, Li RongQing
Cc: seanjc, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, kvm
On 19/10/21 09:23, Vitaly Kuznetsov wrote:
>>
>> -static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
>> +static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu, bool pending)
> Nitpick (and probably a matter of personal taste): pv_eoi_clr_pending()
> has only one user and the change doesn't make its interface much nicer,
> I'd suggest we just inline in instead. (we can probably do the same to
> pv_eoi_get_pending()/pv_eoi_set_pending() too).
Alternatively, merge pv_eoi_get_pending and pv_eoi_clr_pending into a
single function pv_eoi_test_and_clear_pending, which returns the value
of the pending bit.
So the caller can do essentially:
- pending = pv_eoi_get_pending(vcpu);
- pv_eoi_clr_pending(vcpu);
- if (pending)
+ if (pv_eoi_test_and_clear_pending(vcpu))
return;
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] KVM: Clear pv eoi pending bit only when it is set
2021-10-19 7:29 ` Paolo Bonzini
@ 2021-10-20 3:36 ` Li,Rongqing
2021-10-20 11:15 ` Li,Rongqing
1 sibling, 0 replies; 6+ messages in thread
From: Li,Rongqing @ 2021-10-20 3:36 UTC (permalink / raw)
To: Paolo Bonzini, Vitaly Kuznetsov
Cc: seanjc, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, kvm
> -----邮件原件-----
> 发件人: Paolo Bonzini <pbonzini@redhat.com>
> 发送时间: 2021年10月19日 15:29
> 收件人: Vitaly Kuznetsov <vkuznets@redhat.com>; Li,Rongqing
> <lirongqing@baidu.com>
> 抄送: seanjc@google.com; wanpengli@tencent.com; jmattson@google.com;
> joro@8bytes.org; tglx@linutronix.de; mingo@redhat.com; bp@alien8.de;
> x86@kernel.org; hpa@zytor.com; kvm@vger.kernel.org
> 主题: Re: [PATCH] KVM: Clear pv eoi pending bit only when it is set
>
> On 19/10/21 09:23, Vitaly Kuznetsov wrote:
> >>
> >> -static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
> >> +static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu, bool pending)
> > Nitpick (and probably a matter of personal taste):
> > pv_eoi_clr_pending() has only one user and the change doesn't make its
> > interface much nicer, I'd suggest we just inline in instead. (we can
> > probably do the same to
> > pv_eoi_get_pending()/pv_eoi_set_pending() too).
>
> Alternatively, merge pv_eoi_get_pending and pv_eoi_clr_pending into a single
> function pv_eoi_test_and_clear_pending, which returns the value of the
> pending bit.
>
> So the caller can do essentially:
>
> - pending = pv_eoi_get_pending(vcpu);
> - pv_eoi_clr_pending(vcpu);
> - if (pending)
> + if (pv_eoi_test_and_clear_pending(vcpu))
> return;
>
>
It is better to implement pv_eoi_test_and_clear_pending(), and it can fix the race that Vitaly suggested
And I will write a new function kvm_test_and_clear_bit_guest_cached, to be called in pv_eoi_test_and_clear_pending
Bool kvm_test_and_clear_bit_guest_cached（struct kvm_vcpu *vcpu， struct gfn_to_hva_cache * ghc， long nr）
-Li
> Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] KVM: Clear pv eoi pending bit only when it is set
2021-10-19 7:23 ` Vitaly Kuznetsov
2021-10-19 7:29 ` Paolo Bonzini
@ 2021-10-20 3:43 ` Li,Rongqing
1 sibling, 0 replies; 6+ messages in thread
From: Li,Rongqing @ 2021-10-20 3:43 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: pbonzini, seanjc, wanpengli, jmattson, joro, tglx, mingo, bp,
x86, hpa, kvm
> -----邮件原件-----
> 发件人: Vitaly Kuznetsov <vkuznets@redhat.com>
> 发送时间: 2021年10月19日 15:24
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: Li,Rongqing <lirongqing@baidu.com>; pbonzini@redhat.com;
> seanjc@google.com; wanpengli@tencent.com; jmattson@google.com;
> joro@8bytes.org; tglx@linutronix.de; mingo@redhat.com; bp@alien8.de;
> x86@kernel.org; hpa@zytor.com; kvm@vger.kernel.org
> 主题: Re: [PATCH] KVM: Clear pv eoi pending bit only when it is set
>
> Li RongQing <lirongqing@baidu.com> writes:
>
> > clear pv eoi pending bit only when it is set, to avoid calling
> > pv_eoi_put_user()
> >
> > and this can speed pv_eoi_clr_pending about 300 nsec on AMD EPYC most
> > of the time
> >
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > arch/x86/kvm/lapic.c | 7 ++++---
> > 1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index
> > 76fb009..c434f70 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -694,9 +694,9 @@ static void pv_eoi_set_pending(struct kvm_vcpu
> *vcpu)
> > __set_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention); }
> >
> > -static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
> > +static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu, bool pending)
>
> Nitpick (and probably a matter of personal taste): pv_eoi_clr_pending() has only
> one user and the change doesn't make its interface much nicer, I'd suggest we
> just inline in instead. (we can probably do the same to
> pv_eoi_get_pending()/pv_eoi_set_pending() too).
>
> > {
> > - if (pv_eoi_put_user(vcpu, KVM_PV_EOI_DISABLED) < 0) {
> > + if (pending && pv_eoi_put_user(vcpu, KVM_PV_EOI_DISABLED) < 0) {
> > printk(KERN_WARNING "Can't clear EOI MSR value: 0x%llx\n",
> > (unsigned long long)vcpu->arch.pv_eoi.msr_val);
> > return;
> > @@ -2693,7 +2693,8 @@ static void apic_sync_pv_eoi_from_guest(struct
> kvm_vcpu *vcpu,
> > * While this might not be ideal from performance point of view,
> > * this makes sure pv eoi is only enabled when we know it's safe.
> > */
> > - pv_eoi_clr_pending(vcpu);
> > + pv_eoi_clr_pending(vcpu, pending);
> > +
> > if (pending)
> > return;
> > vector = apic_set_eoi(apic);
>
> Could you probably elaborate a bit (probably by enhancing the comment above
> pv_eoi_clr_pending()) why the race we have here (even before the
> patch) doesn't matter? As far as I understand it, the guest can change PV EOI
> status from a different CPU (it shouldn't do it but it still can) at any time: e.g.
> between pv_eoi_get_pending() and pv_eoi_clr_pending() but also right after we
> do pv_eoi_clr_pending() so the patch doesn't really change much in this regard.
>
Is it reasonable that the guest change PV EOI status from a different CPU? I think this can lead to guest error or stuck
And new function pv_eoi_test_and_clear_pending and kvm_test_and_clear_bit_guest_cached should be able to fix the race
I will send V2
Thanks
-Li
> --
> Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] KVM: Clear pv eoi pending bit only when it is set
2021-10-19 7:29 ` Paolo Bonzini
2021-10-20 3:36 ` 答复: " Li,Rongqing
@ 2021-10-20 11:15 ` Li,Rongqing
1 sibling, 0 replies; 6+ messages in thread
From: Li,Rongqing @ 2021-10-20 11:15 UTC (permalink / raw)
To: Paolo Bonzini, Vitaly Kuznetsov
Cc: seanjc, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, kvm
> >
> > On 19/10/21 09:23, Vitaly Kuznetsov wrote:
> > >>
> > >> -static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
> > >> +static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu, bool
> > >> +pending)
> > > Nitpick (and probably a matter of personal taste):
> > > pv_eoi_clr_pending() has only one user and the change doesn't make
> > > its interface much nicer, I'd suggest we just inline in instead. (we
> > > can probably do the same to
> > > pv_eoi_get_pending()/pv_eoi_set_pending() too).
> >
> > Alternatively, merge pv_eoi_get_pending and pv_eoi_clr_pending into a
> > single function pv_eoi_test_and_clear_pending, which returns the value
> > of the pending bit.
> >
> > So the caller can do essentially:
> >
> > - pending = pv_eoi_get_pending(vcpu);
> > - pv_eoi_clr_pending(vcpu);
> > - if (pending)
> > + if (pv_eoi_test_and_clear_pending(vcpu))
> > return;
> >
> >
>
> It is better to implement pv_eoi_test_and_clear_pending(), and it can fix the
> race that Vitaly suggested
>
> And I will write a new function kvm_test_and_clear_bit_guest_cached, to be
> called in pv_eoi_test_and_clear_pending
>
> Bool kvm_test_and_clear_bit_guest_cached（struct kvm_vcpu *vcpu， struct
> gfn_to_hva_cache * ghc， long nr）
>
gfn_to_hva_cache has hva(user space address), but no hpa, and test_and_clear() can not be used to user space address. So kvm_test_and_clear_bit_guest_cached seems not work
-Li
> -Li
>
> > Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-20 11:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 2:05 [PATCH] KVM: Clear pv eoi pending bit only when it is set Li RongQing
2021-10-19 7:23 ` Vitaly Kuznetsov
2021-10-19 7:29 ` Paolo Bonzini
2021-10-20 3:36 ` 答复: " Li,Rongqing
2021-10-20 11:15 ` Li,Rongqing
2021-10-20 3:43 ` Li,Rongqing
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.