[PATCH v2] x86/kvm: Use RCU for svm_vm_data_hash

[PATCH v2] x86/kvm: Use RCU for svm_vm_data_hash

[Sairaj Kodilkar]

GALOG notifier iterates svm_vm_data_hash to find the `struct kvm_vcpu`
for the given vm id and vcpu id. Holding spinlock is not ideal here since
it allows only a single reader to iterate over the hash. This degrades
performance when interrupt rate is high and large number of readers are
waiting for the spinlock. Hence replace spinlock with the RCU.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---

Tested on a 2-socket AMD EPYC 9965 host against the same kernel without
this change:

  - 12 VMs, each with 32 vCPUs and one passed-through NVMe device
  - vCPUs pinned NUMA-local, at 1:1, 1:2, 1:3 and 1:4 pCPU:vCPU
  - fio in each guest: randread, bs=4k, iodepth=32, numjobs=32,
    ioengine=libaio, direct=1, 60s

Time spent in the hash lookup in avic_ga_log_notifier(), measured with
a temporary tracepoint around the lookup, averaged over 12 VMs and
3 runs:

             |         mean           |          p99         |
             |------------------------|----------------------|
  pCPU:vCPU  | spinlock  |     RCU    | spinlock  |     RCU  |
-------------|-----------|------------|-----------|----------|
  1:1        | 409.8 ns  |  132.6 ns  |   960 ns  |  750 ns  |
  1:2        | 977.3 ns  |   21.9 ns  |  3530 ns  |   30 ns  |
  1:3        | 747.5 ns  |   23.7 ns  |  2150 ns  |   40 ns  |
  1:4        | 614.8 ns  |   23.5 ns  |  1610 ns  |   40 ns  |

Total IOPS was unchanged at every ratio.

Changes since v1:
================
https://lore.kernel.org/all/a2a4c5e6-8264-47e1-80de-375dc2eac048@amd.com/
- Restore spinlock guards.
- Use synchronize_rcu() in destruction path.

---
 arch/x86/kvm/svm/avic.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 96ca39c0045f..84a0e8d406e4 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -250,7 +250,6 @@ static void avic_deactivate_vmcb(struct vcpu_svm *svm)
  */
 static int avic_ga_log_notifier(u32 ga_tag)
 {
-	unsigned long flags;
 	struct kvm_svm *kvm_svm;
 	struct kvm_vcpu *vcpu = NULL;
 	u32 vm_id = AVIC_GATAG_TO_VMID(ga_tag);
@@ -259,14 +258,13 @@ static int avic_ga_log_notifier(u32 ga_tag)
 	pr_debug("SVM: %s: vm_id=%#x, vcpu_idx=%#x\n", __func__, vm_id, vcpu_idx);
 	trace_kvm_avic_ga_log(vm_id, vcpu_idx);
 
-	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
-	hash_for_each_possible(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
+	rcu_read_lock();
+	hash_for_each_possible_rcu(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
 		if (kvm_svm->avic_vm_id != vm_id)
 			continue;
 		vcpu = kvm_get_vcpu(&kvm_svm->kvm, vcpu_idx);
 		break;
 	}
-	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
 
 	/* Note:
 	 * At this point, the IOMMU should have already set the pending
@@ -276,6 +274,8 @@ static int avic_ga_log_notifier(u32 ga_tag)
 	if (vcpu)
 		kvm_vcpu_wake_up(vcpu);
 
+	rcu_read_unlock();
+
 	return 0;
 }
 
@@ -340,7 +340,7 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
 		}
 	}
 	kvm_svm->avic_vm_id = vm_id;
-	hash_add(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
+	hash_add_rcu(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
 }
 
 int avic_vcpu_precreate(struct kvm *kvm)
@@ -373,9 +373,12 @@ void avic_vm_pre_destroy(struct kvm *kvm)
 	if (WARN_ON_ONCE(!enable_apicv) || !kvm_svm->avic_vm_id)
 		return;
 
-	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
+	{
+		guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
+		hash_del_rcu(&kvm_svm->hnode);
+	}
 
-	hash_del(&kvm_svm->hnode);
+	synchronize_rcu();
 }
 
 void avic_vm_destroy(struct kvm *kvm)
-- 
2.34.1

Re: [PATCH v2] x86/kvm: Use RCU for svm_vm_data_hash

[Sean Christopherson]

KVM: SVM:

"x86/kvm:" is specifically for guest-side code, e.g. arch/x86/kernel/kvm.c

> Changes since v1:

In the future, please wait the "normal" amount of time before sending a new
version, e.g. a day or two for something relatively simple like this.  I.e.
treat feedback from Sashiko like feedback from a human reviewer, and don't spam
patches.

There are times when it's acceptable to break the "don't spam patches" rule, but
this isn't one of those times.

>  int avic_vcpu_precreate(struct kvm *kvm)
> @@ -373,9 +373,12 @@ void avic_vm_pre_destroy(struct kvm *kvm)
>  	if (WARN_ON_ONCE(!enable_apicv) || !kvm_svm->avic_vm_id)
>  		return;
>  
> -	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
> +	{
> +		guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
> +		hash_del_rcu(&kvm_svm->hnode);

Eww.  Don't open code scoped_guard(), e.g. this should work:

	scoped_guard(spinlock_irqsave, &svm_vm_data_hash_lock)
		hash_del(&kvm_svm->hnode);

	synchronize_rcu();

I'm a little hesitant to put a full synchronize_rcu() in the destroy path, but
in practice it's should be ok.

Re: [PATCH v2] x86/kvm: Use RCU for svm_vm_data_hash

[Sairaj Kodilkar]

On 11/11/2026 8:34 PM, Sean Christopherson wrote:
> KVM: SVM:
> 
> "x86/kvm:" is specifically for guest-side code, e.g. arch/x86/kernel/kvm.c
> 
>> Changes since v1:
> 
> In the future, please wait the "normal" amount of time before sending a new
> version, e.g. a day or two for something relatively simple like this.  I.e.
> treat feedback from Sashiko like feedback from a human reviewer, and don't spam
> patches.
> 
> There are times when it's acceptable to break the "don't spam patches" rule, but
> this isn't one of those times.

Hi Sean

Sorry about this, did not mean to spam the patches. I accidently sent
wrong version upstream, thats why decided to respin it. I'll be careful
from next time onwards.

> 
>>  int avic_vcpu_precreate(struct kvm *kvm)
>> @@ -373,9 +373,12 @@ void avic_vm_pre_destroy(struct kvm *kvm)
>>  	if (WARN_ON_ONCE(!enable_apicv) || !kvm_svm->avic_vm_id)
>>  		return;
>>  
>> -	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
>> +	{
>> +		guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
>> +		hash_del_rcu(&kvm_svm->hnode);
> 
> Eww.  Don't open code scoped_guard(), e.g. this should work:

Ahh right, I missed that.

> 
> 	scoped_guard(spinlock_irqsave, &svm_vm_data_hash_lock)
> 		hash_del(&kvm_svm->hnode);
> 
> 	synchronize_rcu();
> 
> I'm a little hesitant to put a full synchronize_rcu() in the destroy path, but
> in practice it's should be ok.

Thanks
Sairaj