| Message ID | 20211009021236.4122790-36-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Halt-polling and x86 APICv overhaul | expand |
On 09/10/21 04:12, Sean Christopherson wrote: > + */ > + if (vcpu->mode == IN_GUEST_MODE) { > int cpu = READ_ONCE(vcpu->cpu); > > /* > @@ -687,8 +692,13 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) > if (cpu != get_cpu()) > wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu)); > put_cpu(); > - } else > + } else { > + /* > + * Wake the vCPU if it was blocking. KVM will then detect the > + * pending IRQ when checking if the vCPU has a wake event. > + */ > kvm_vcpu_wake_up(vcpu); > + } > Does this still need to check the "running" flag? That should be a strict superset of vcpu->mode == IN_GUEST_MODE. Paolo
On Mon, Oct 25, 2021, Paolo Bonzini wrote: > On 09/10/21 04:12, Sean Christopherson wrote: > > + */ > > + if (vcpu->mode == IN_GUEST_MODE) { > > int cpu = READ_ONCE(vcpu->cpu); > > /* > > @@ -687,8 +692,13 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) > > if (cpu != get_cpu()) > > wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu)); > > put_cpu(); > > - } else > > + } else { > > + /* > > + * Wake the vCPU if it was blocking. KVM will then detect the > > + * pending IRQ when checking if the vCPU has a wake event. > > + */ > > kvm_vcpu_wake_up(vcpu); > > + } > > Does this still need to check the "running" flag? That should be a strict > superset of vcpu->mode == IN_GUEST_MODE. No. Signalling the doorbell when "running" is set but the vCPU is not in the guest is just an expensive nop. So even if KVM were to rework its handling of "running" to set the flag immediately before VMRUN and clear it immediately after, keying off IN_GUEST_MODE and not "running" would not be wrong, just sub-optimal. I doubt KVM will ever make the "running" flag super precise, because keeping the flag set when the vCPU is loaded avoids VM-Exits on other vCPUs due to undelivered IPIs. But the flip side is that it means the flag has terrible granularity, and is arguably inaccurate when viewed from a software perspective. Anyways, if the treatment of "running" were ever changed, then this code should also be changed to essentially revert this commit since vcpu->mode would then be redundant. And IMO, it makes sense to intentionally separate KVM's delivery of interrupts from hardware's delivery of interrupts. I.e. use the same core rules as kvm_vcpu_kick() for when to send interrupts and when to wake for the AVIC.
On 27/10/21 17:06, Sean Christopherson wrote: >> Does this still need to check the "running" flag? That should be a strict >> superset of vcpu->mode == IN_GUEST_MODE. > > No. Signalling the doorbell when "running" is set but the vCPU is not in the > guest is just an expensive nop. So even if KVM were to rework its handling of > "running" to set the flag immediately before VMRUN and clear it immediately after, > keying off IN_GUEST_MODE and not "running" would not be wrong, just sub-optimal. > > I doubt KVM will ever make the "running" flag super precise, because keeping the > flag set when the vCPU is loaded avoids VM-Exits on other vCPUs due to undelivered > IPIs. Right, so should we drop the "if (running)" check in this patch, at the same time as it's adding the IN_GUEST_MODE check? Paolo
On Wed, Oct 27, 2021, Paolo Bonzini wrote: > On 27/10/21 17:06, Sean Christopherson wrote: > > > Does this still need to check the "running" flag? That should be a strict > > > superset of vcpu->mode == IN_GUEST_MODE. > > > > No. Signalling the doorbell when "running" is set but the vCPU is not in the > > guest is just an expensive nop. So even if KVM were to rework its handling of > > "running" to set the flag immediately before VMRUN and clear it immediately after, > > keying off IN_GUEST_MODE and not "running" would not be wrong, just sub-optimal. > > > > I doubt KVM will ever make the "running" flag super precise, because keeping the > > flag set when the vCPU is loaded avoids VM-Exits on other vCPUs due to undelivered > > IPIs. > > Right, so should we drop the "if (running)" check in this patch, at the same > time as it's adding the IN_GUEST_MODE check? LOL, I think we have a Three^WTwo Stooges routine going on. This patch does remove avic_vcpu_is_running() and replaces it with the vcpu->mode check. Or am I completely misunderstanding what your referring to? - if (avic_vcpu_is_running(vcpu)) { + /* + * Signal the doorbell to tell hardware to inject the IRQ if the vCPU + * is in the guest. If the vCPU is not in the guest, hardware will + * automatically process AVIC interrupts at VMRUN. + */ + if (vcpu->mode == IN_GUEST_MODE) { int cpu = READ_ONCE(vcpu->cpu);
On 27/10/21 18:08, Sean Christopherson wrote: >> Right, so should we drop the "if (running)" check in this patch, at the same >> time as it's adding the IN_GUEST_MODE check? > LOL, I think we have a Three^WTwo Stooges routine going on. This patch does > remove avic_vcpu_is_running() and replaces it with the vcpu->mode check. Or am > I completely misunderstanding what your referring to? > > - if (avic_vcpu_is_running(vcpu)) { > + /* > + * Signal the doorbell to tell hardware to inject the IRQ if the vCPU > + * is in the guest. If the vCPU is not in the guest, hardware will > + * automatically process AVIC interrupts at VMRUN. > + */ > + if (vcpu->mode == IN_GUEST_MODE) { > int cpu = READ_ONCE(vcpu->cpu); Nevermind, I confused svm_deliver_avic_intr with avic_kick_target_vcpus, which anyway you are handling in patch 36. Paolo
On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > Signal the AVIC doorbell iff the vCPU is running in the guest. If the vCPU > is not IN_GUEST_MODE, it's guaranteed to pick up any pending IRQs on the > next VMRUN, which unconditionally processes the vIRR. > > Add comments to document the logic. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/svm/avic.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > index 208c5c71e827..cbf02e7e20d0 100644 > --- a/arch/x86/kvm/svm/avic.c > +++ b/arch/x86/kvm/svm/avic.c > @@ -674,7 +674,12 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) > kvm_lapic_set_irr(vec, vcpu->arch.apic); > smp_mb__after_atomic(); > > - if (avic_vcpu_is_running(vcpu)) { > + /* > + * Signal the doorbell to tell hardware to inject the IRQ if the vCPU > + * is in the guest. If the vCPU is not in the guest, hardware will > + * automatically process AVIC interrupts at VMRUN. > + */ > + if (vcpu->mode == IN_GUEST_MODE) { > int cpu = READ_ONCE(vcpu->cpu); > > /* > @@ -687,8 +692,13 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) > if (cpu != get_cpu()) > wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu)); > put_cpu(); > - } else > + } else { > + /* > + * Wake the vCPU if it was blocking. KVM will then detect the > + * pending IRQ when checking if the vCPU has a wake event. > + */ > kvm_vcpu_wake_up(vcpu); > + } > > return 0; > } It makes sense indeed to avoid ringing the doorbell when the vCPU is not in the guest mode. I do wonder if we want to call kvm_vcpu_wake_up always otherwise, as the vCPU might be just outside of the guest mode and not scheduled out. I don't know how expensive is kvm_vcpu_wake_up in this case. Before this patch, the avic_vcpu_is_running would only be false when the vCPU is scheduled out (e.g when vcpu_put was done on it) Best regards, Maxim Levitsky
On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Signal the AVIC doorbell iff the vCPU is running in the guest. If the vCPU > > is not IN_GUEST_MODE, it's guaranteed to pick up any pending IRQs on the > > next VMRUN, which unconditionally processes the vIRR. > > > > Add comments to document the logic. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/svm/avic.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > > index 208c5c71e827..cbf02e7e20d0 100644 > > --- a/arch/x86/kvm/svm/avic.c > > +++ b/arch/x86/kvm/svm/avic.c > > @@ -674,7 +674,12 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) > > kvm_lapic_set_irr(vec, vcpu->arch.apic); > > smp_mb__after_atomic(); > > > > - if (avic_vcpu_is_running(vcpu)) { > > + /* > > + * Signal the doorbell to tell hardware to inject the IRQ if the vCPU > > + * is in the guest. If the vCPU is not in the guest, hardware will > > + * automatically process AVIC interrupts at VMRUN. > > + */ > > + if (vcpu->mode == IN_GUEST_MODE) { > > int cpu = READ_ONCE(vcpu->cpu); > > > > /* > > @@ -687,8 +692,13 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) > > if (cpu != get_cpu()) > > wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu)); > > put_cpu(); > > - } else > > + } else { > > + /* > > + * Wake the vCPU if it was blocking. KVM will then detect the > > + * pending IRQ when checking if the vCPU has a wake event. > > + */ > > kvm_vcpu_wake_up(vcpu); > > + } > > > > return 0; > > } > > It makes sense indeed to avoid ringing the doorbell when the vCPU is not in > the guest mode. > > I do wonder if we want to call kvm_vcpu_wake_up always otherwise, as the vCPU > might be just outside of the guest mode and not scheduled out. I don't know > how expensive is kvm_vcpu_wake_up in this case. IIUC, you're asking if we should do something like: if (vcpu->mode == IN_GUEST_MODE) { <signal doorbell> } else if (!is_vcpu_loaded(vcpu)) { kvm_vcpu_wake_up(); } The answer is that kvm_vcpu_wake_up(), which is effectively rcuwait_wake_up(), is very cheap except for specific configurations that may or may not be valid for production[*]. Practically speaking, is_vcpu_loaded() doesn't exist and should never exist because it's inherently racy. The closest we have would be else if (vcpu != kvm_get_running_vcpu()) { kvm_vcpu_wake_up(); } but that's extremely unlikely to be a net win because getting the current vCPU requires atomics to disable/re-enable preemption, especially if rcuwait_wake_up() is modified to avoid the rcu lock/unlock. TL;DR: rcuwait_wake_up() is cheap, and if it's too expensive, a better optimization would be to make it less expensive. [*] https://lkml.kernel.org/r/20211020110638.797389-1-pbonzini@redhat.com > Before this patch, the avic_vcpu_is_running would only be false when the vCPU > is scheduled out (e.g when vcpu_put was done on it) > > Best regards, > Maxim Levitsky >
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index 208c5c71e827..cbf02e7e20d0 100644 --- a/arch/x86/kvm/svm/avic.c +++ b/arch/x86/kvm/svm/avic.c @@ -674,7 +674,12 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) kvm_lapic_set_irr(vec, vcpu->arch.apic); smp_mb__after_atomic(); - if (avic_vcpu_is_running(vcpu)) { + /* + * Signal the doorbell to tell hardware to inject the IRQ if the vCPU + * is in the guest. If the vCPU is not in the guest, hardware will + * automatically process AVIC interrupts at VMRUN. + */ + if (vcpu->mode == IN_GUEST_MODE) { int cpu = READ_ONCE(vcpu->cpu); /* @@ -687,8 +692,13 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) if (cpu != get_cpu()) wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu)); put_cpu(); - } else + } else { + /* + * Wake the vCPU if it was blocking. KVM will then detect the + * pending IRQ when checking if the vCPU has a wake event. + */ kvm_vcpu_wake_up(vcpu); + } return 0; }
Signal the AVIC doorbell iff the vCPU is running in the guest. If the vCPU is not IN_GUEST_MODE, it's guaranteed to pick up any pending IRQs on the next VMRUN, which unconditionally processes the vIRR. Add comments to document the logic. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/avic.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)