| Message ID | 3d8ed6be41358c7635bd4e09ecdfd1bc77ce83df.1738595289.git.naveen@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: x86: Address performance degradation due to APICv inhibits | expand |
On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote: > apicv_update_lock is primarily meant for protecting updates to the apicv > state, and is not necessary for guarding updates to synic_auto_eoi_used. > Convert synic_auto_eoi_used to an atomic and use > kvm_set_or_clear_apicv_inhibit() helper to simplify the logic. > > Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org> > --- > arch/x86/include/asm/kvm_host.h | 7 ++----- > arch/x86/kvm/hyperv.c | 17 +++++------------ > 2 files changed, 7 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 5193c3dfbce1..fb93563714c2 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1150,11 +1150,8 @@ struct kvm_hv { > /* How many vCPUs have VP index != vCPU index */ > atomic_t num_mismatched_vp_indexes; > > - /* > - * How many SynICs use 'AutoEOI' feature > - * (protected by arch.apicv_update_lock) > - */ > - unsigned int synic_auto_eoi_used; > + /* How many SynICs use 'AutoEOI' feature */ > + atomic_t synic_auto_eoi_used; > > struct kvm_hv_syndbg hv_syndbg; > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 6a6dd5a84f22..7a4554ea1d16 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > if (auto_eoi_old == auto_eoi_new) > return; > > - if (!enable_apicv) > - return; > - > - down_write(&vcpu->kvm->arch.apicv_update_lock); > - > if (auto_eoi_new) > - hv->synic_auto_eoi_used++; > + atomic_inc(&hv->synic_auto_eoi_used); > else > - hv->synic_auto_eoi_used--; > + atomic_dec(&hv->synic_auto_eoi_used); > > /* > * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on > * the hypervisor to manually inject IRQs. > */ > - __kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > - APICV_INHIBIT_REASON_HYPERV, > - !!hv->synic_auto_eoi_used); > - > - up_write(&vcpu->kvm->arch.apicv_update_lock); > + kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > + APICV_INHIBIT_REASON_HYPERV, > + !!atomic_read(&hv->synic_auto_eoi_used)); Hi, This introduces a race, because there is a race window between the moment we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit. After we read hv->synic_auto_eoi_used, but before we call the kvm_set_or_clear_apicv_inhibit, other core might also run synic_update_vector and change hv->synic_auto_eoi_used, finish setting the inhibit in kvm_set_or_clear_apicv_inhibit, and only then we will call kvm_set_or_clear_apicv_inhibit with the stale value of hv->synic_auto_eoi_used and clear it. IMHO, knowing that this code is mostly a precaution and that modern windows doesn't use AutoEOI (at least when AutoEOI deprecation bit is set), instead of counting, we can unconditionally inhibit the APICv when the guest attempts to use AutoEOI once. But as usual I won't be surprised that this breaks *some* old and/or odd windows versions. Best regards, Maxim Levitsky > } > > static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
Hi Maxim, On Mon, Feb 03, 2025 at 08:30:13PM -0500, Maxim Levitsky wrote: > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote: > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > > index 6a6dd5a84f22..7a4554ea1d16 100644 > > --- a/arch/x86/kvm/hyperv.c > > +++ b/arch/x86/kvm/hyperv.c > > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > > if (auto_eoi_old == auto_eoi_new) > > return; > > > > - if (!enable_apicv) > > - return; > > - > > - down_write(&vcpu->kvm->arch.apicv_update_lock); > > - > > if (auto_eoi_new) > > - hv->synic_auto_eoi_used++; > > + atomic_inc(&hv->synic_auto_eoi_used); > > else > > - hv->synic_auto_eoi_used--; > > + atomic_dec(&hv->synic_auto_eoi_used); > > > > /* > > * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on > > * the hypervisor to manually inject IRQs. > > */ > > - __kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > > - APICV_INHIBIT_REASON_HYPERV, > > - !!hv->synic_auto_eoi_used); > > - > > - up_write(&vcpu->kvm->arch.apicv_update_lock); > > + kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > > + APICV_INHIBIT_REASON_HYPERV, > > + !!atomic_read(&hv->synic_auto_eoi_used)); > > Hi, > > This introduces a race, because there is a race window between > the moment we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit. > > After we read hv->synic_auto_eoi_used, but before we call the kvm_set_or_clear_apicv_inhibit, > other core might also run synic_update_vector and change hv->synic_auto_eoi_used, > finish setting the inhibit in kvm_set_or_clear_apicv_inhibit, > and only then we will call kvm_set_or_clear_apicv_inhibit with the stale value of hv->synic_auto_eoi_used and clear it. Ah, indeed. Thanks for the explanation. I wonder if we can switch to using kvm_hv->hv_lock in place of apicv_update_lock. That lock is already used to guard updates to partition-wide MSRs in kvm_hv_set_msr_common(). So, that might be ok too? Thanks, Naveen
On Tue, Feb 04, 2025, Naveen N Rao wrote: > Hi Maxim, > > On Mon, Feb 03, 2025 at 08:30:13PM -0500, Maxim Levitsky wrote: > > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote: > > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > > > index 6a6dd5a84f22..7a4554ea1d16 100644 > > > --- a/arch/x86/kvm/hyperv.c > > > +++ b/arch/x86/kvm/hyperv.c > > > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > > > if (auto_eoi_old == auto_eoi_new) > > > return; > > > > > > - if (!enable_apicv) > > > - return; > > > - > > > - down_write(&vcpu->kvm->arch.apicv_update_lock); > > > - > > > if (auto_eoi_new) > > > - hv->synic_auto_eoi_used++; > > > + atomic_inc(&hv->synic_auto_eoi_used); > > > else > > > - hv->synic_auto_eoi_used--; > > > + atomic_dec(&hv->synic_auto_eoi_used); > > > > > > /* > > > * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on > > > * the hypervisor to manually inject IRQs. > > > */ > > > - __kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > > > - APICV_INHIBIT_REASON_HYPERV, > > > - !!hv->synic_auto_eoi_used); > > > - > > > - up_write(&vcpu->kvm->arch.apicv_update_lock); > > > + kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > > > + APICV_INHIBIT_REASON_HYPERV, > > > + !!atomic_read(&hv->synic_auto_eoi_used)); > > > > Hi, > > > > This introduces a race, because there is a race window between the moment > > we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit. > > > > After we read hv->synic_auto_eoi_used, but before we call the > > kvm_set_or_clear_apicv_inhibit, other core might also run > > synic_update_vector and change hv->synic_auto_eoi_used, finish setting the > > inhibit in kvm_set_or_clear_apicv_inhibit, and only then we will call > > kvm_set_or_clear_apicv_inhibit with the stale value of > > hv->synic_auto_eoi_used and clear it. > > Ah, indeed. Thanks for the explanation. > > I wonder if we can switch to using kvm_hv->hv_lock in place of > apicv_update_lock. That lock is already used to guard updates to > partition-wide MSRs in kvm_hv_set_msr_common(). So, that might be ok > too? Why? All that would do is add complexity (taking two locks, or ensuring there is no race when juggling locks), because if the guest is actually toggling AutoEOI at a meaningful rate on multiple vCPUs, then there is going to be lock contention regardless of which lock is taken.
On Tue, Feb 04, 2025 at 11:33:01AM -0800, Sean Christopherson wrote: > On Tue, Feb 04, 2025, Naveen N Rao wrote: > > Hi Maxim, > > > > On Mon, Feb 03, 2025 at 08:30:13PM -0500, Maxim Levitsky wrote: > > > On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote: > > > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > > > > index 6a6dd5a84f22..7a4554ea1d16 100644 > > > > --- a/arch/x86/kvm/hyperv.c > > > > +++ b/arch/x86/kvm/hyperv.c > > > > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > > > > if (auto_eoi_old == auto_eoi_new) > > > > return; > > > > > > > > - if (!enable_apicv) > > > > - return; > > > > - > > > > - down_write(&vcpu->kvm->arch.apicv_update_lock); > > > > - > > > > if (auto_eoi_new) > > > > - hv->synic_auto_eoi_used++; > > > > + atomic_inc(&hv->synic_auto_eoi_used); > > > > else > > > > - hv->synic_auto_eoi_used--; > > > > + atomic_dec(&hv->synic_auto_eoi_used); > > > > > > > > /* > > > > * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on > > > > * the hypervisor to manually inject IRQs. > > > > */ > > > > - __kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > > > > - APICV_INHIBIT_REASON_HYPERV, > > > > - !!hv->synic_auto_eoi_used); > > > > - > > > > - up_write(&vcpu->kvm->arch.apicv_update_lock); > > > > + kvm_set_or_clear_apicv_inhibit(vcpu->kvm, > > > > + APICV_INHIBIT_REASON_HYPERV, > > > > + !!atomic_read(&hv->synic_auto_eoi_used)); > > > > > > Hi, > > > > > > This introduces a race, because there is a race window between the moment > > > we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit. > > > > > > After we read hv->synic_auto_eoi_used, but before we call the > > > kvm_set_or_clear_apicv_inhibit, other core might also run > > > synic_update_vector and change hv->synic_auto_eoi_used, finish setting the > > > inhibit in kvm_set_or_clear_apicv_inhibit, and only then we will call > > > kvm_set_or_clear_apicv_inhibit with the stale value of > > > hv->synic_auto_eoi_used and clear it. > > > > Ah, indeed. Thanks for the explanation. > > > > I wonder if we can switch to using kvm_hv->hv_lock in place of > > apicv_update_lock. That lock is already used to guard updates to > > partition-wide MSRs in kvm_hv_set_msr_common(). So, that might be ok > > too? > > Why? All that would do is add complexity (taking two locks, or ensuring there > is no race when juggling locks), because if the guest is actually > toggling AutoEOI > at a meaningful rate on multiple vCPUs, then there is going to be lock contention > regardless of which lock is taken. Yes, indeed. The rationale for switching to a different lock was to address the original goal with this patch, which is to restrict use of apicv_update_lock to only toggling the APICv state. But, that is only relevant if we want to attempt that. I do see why hv_lock won't work in this scenario though, so yes, we either need to retain use of apicv_update_lock, or introduce a new mutex for protecting updates to synic_auto_eoi_used. Thanks, Naveen
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 5193c3dfbce1..fb93563714c2 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1150,11 +1150,8 @@ struct kvm_hv { /* How many vCPUs have VP index != vCPU index */ atomic_t num_mismatched_vp_indexes; - /* - * How many SynICs use 'AutoEOI' feature - * (protected by arch.apicv_update_lock) - */ - unsigned int synic_auto_eoi_used; + /* How many SynICs use 'AutoEOI' feature */ + atomic_t synic_auto_eoi_used; struct kvm_hv_syndbg hv_syndbg; diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 6a6dd5a84f22..7a4554ea1d16 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, if (auto_eoi_old == auto_eoi_new) return; - if (!enable_apicv) - return; - - down_write(&vcpu->kvm->arch.apicv_update_lock); - if (auto_eoi_new) - hv->synic_auto_eoi_used++; + atomic_inc(&hv->synic_auto_eoi_used); else - hv->synic_auto_eoi_used--; + atomic_dec(&hv->synic_auto_eoi_used); /* * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on * the hypervisor to manually inject IRQs. */ - __kvm_set_or_clear_apicv_inhibit(vcpu->kvm, - APICV_INHIBIT_REASON_HYPERV, - !!hv->synic_auto_eoi_used); - - up_write(&vcpu->kvm->arch.apicv_update_lock); + kvm_set_or_clear_apicv_inhibit(vcpu->kvm, + APICV_INHIBIT_REASON_HYPERV, + !!atomic_read(&hv->synic_auto_eoi_used)); } static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
apicv_update_lock is primarily meant for protecting updates to the apicv state, and is not necessary for guarding updates to synic_auto_eoi_used. Convert synic_auto_eoi_used to an atomic and use kvm_set_or_clear_apicv_inhibit() helper to simplify the logic. Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org> --- arch/x86/include/asm/kvm_host.h | 7 ++----- arch/x86/kvm/hyperv.c | 17 +++++------------ 2 files changed, 7 insertions(+), 17 deletions(-)