| Message ID | 1634609144-28952-1-git-send-email-lirongqing@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Clear pv eoi pending bit only when it is set | expand |
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.
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
> -----邮件原件----- > 发件人: 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
> -----邮件原件----- > 发件人: 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
> > > > 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
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);
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(-)