KVM: x86: Convert tsc_write_lock to raw_spinlock

Code under this lock requires non-preemptibility. Ensure this also over -rt by converting it to raw spinlock.

Quoted 3 lines · Jan Kiszka

Code under this lock requires non-preemptibility. Ensure this also over -rt by converting it to raw spinlock.

Oh dear, I had forgotten about that. I believe kvm_lock might have the same assumption in a few places regarding clock.

Quoted 9 lines · Zachary Amsden

Quoted 4 lines · Jan Kiszka

Code under this lock requires non-preemptibility. Ensure this also over -rt by converting it to raw spinlock.

Oh dear, I had forgotten about that. I believe kvm_lock might have the same assumption in a few places regarding clock.

I only found a problematic section in kvmclock_cpufreq_notifier. Didn't see this during my tests as I have CPUFREQ disabled in my .config.

We may need something like this as converting kvm_lock would likely be overkill:

arch/x86/kvm/x86.c +4 -2

index 36f54fb..971ee0d 100644
@@ -4530,7 +4530,7 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
  	struct cpufreq_freqs *freq = data;
  	struct kvm *kvm;
  	struct kvm_vcpu *vcpu;
-	int i, send_ipi = 0;
+	int i, me, send_ipi = 0;
 
  	/*
  	 * We allow guests to temporarily run on slowing clocks,

@@ -4583,9 +4583,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
  		kvm_for_each_vcpu(i, vcpu, kvm) {
  			if (vcpu->cpu != freq->cpu)
  				continue;
+			me = get_cpu();
  			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
-			if (vcpu->cpu != smp_processor_id())
+			if (vcpu->cpu != me)
  				send_ipi = 1;
+			put_cpu();
  		}
  	}
  	spin_unlock(&kvm_lock);

Jan

Quoted 44 lines · Jan Kiszka

Quoted 9 lines · Zachary Amsden

Quoted 4 lines · Jan Kiszka

Code under this lock requires non-preemptibility. Ensure this also over -rt by converting it to raw spinlock.

Oh dear, I had forgotten about that. I believe kvm_lock might have the same assumption in a few places regarding clock.

I only found a problematic section in kvmclock_cpufreq_notifier. Didn't see this during my tests as I have CPUFREQ disabled in my .config.

We may need something like this as converting kvm_lock would likely be overkill:

arch/x86/kvm/x86.c +4 -2

index 36f54fb..971ee0d 100644
@@ -4530,7 +4530,7 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
  	struct cpufreq_freqs *freq = data;
  	struct kvm *kvm;
  	struct kvm_vcpu *vcpu;
-	int i, send_ipi = 0;
+	int i, me, send_ipi = 0;
 
  	/*
  	 * We allow guests to temporarily run on slowing clocks,

@@ -4583,9 +4583,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
  		kvm_for_each_vcpu(i, vcpu, kvm) {
  			if (vcpu->cpu != freq->cpu)
  				continue;
+			me = get_cpu();
  			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
-			if (vcpu->cpu != smp_processor_id())
+			if (vcpu->cpu != me)
  				send_ipi = 1;
+			put_cpu();
  		}
  	}
  	spin_unlock(&kvm_lock);

Quoted 3 lines

Jan

That looks like a good solution, and I do believe that is the only place the lock is used in that fashion - please add a comment though in the giant comment block above that preemption protection is needed for RT. Also, gcc should catch this, but moving the me variable into the kvm_for_each_vcpu loop should allow for better register allocation.

The only other thing I can think of is that RT lock preemption may break some of the CPU initialization semantics enforced by kvm_lock if you happen to get a hotplug event just as the module is loading. That should be rare, but if it is indeed a bug, it would be nice to fix, it would be a panic for sure not to initialize VMX.

Quoted 60 lines · Zachary Amsden

Quoted 48 lines · Jan Kiszka

Quoted 12 lines · Zachary Amsden

Quoted 5 lines · Jan Kiszka

Code under this lock requires non-preemptibility. Ensure this also over -rt by converting it to raw spinlock.

Oh dear, I had forgotten about that. I believe kvm_lock might have the same assumption in a few places regarding clock.

I only found a problematic section in kvmclock_cpufreq_notifier. Didn't see this during my tests as I have CPUFREQ disabled in my .config.

We may need something like this as converting kvm_lock would likely be overkill:

arch/x86/kvm/x86.c +4 -2

index 36f54fb..971ee0d 100644
@@ -4530,7 +4530,7 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
   	struct cpufreq_freqs *freq = data;
   	struct kvm *kvm;
   	struct kvm_vcpu *vcpu;
-	int i, send_ipi = 0;
+	int i, me, send_ipi = 0;
 
   	/*
   	 * We allow guests to temporarily run on slowing clocks,

@@ -4583,9 +4583,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
   		kvm_for_each_vcpu(i, vcpu, kvm) {
   			if (vcpu->cpu != freq->cpu)
   				continue;
+			me = get_cpu();
   			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
-			if (vcpu->cpu != smp_processor_id())
+			if (vcpu->cpu != me)
   				send_ipi = 1;
+			put_cpu();
   		}
   	}
   	spin_unlock(&kvm_lock);

Quoted 4 lines

Jan

That looks like a good solution, and I do believe that is the only place the lock is used in that fashion - please add a comment though in the giant comment block above that preemption protection is needed for RT. Also, gcc should catch this, but moving the me variable into the kvm_for_each_vcpu loop should allow for better register allocation.

Quoted 6 lines

The only other thing I can think of is that RT lock preemption may break some of the CPU initialization semantics enforced by kvm_lock if you happen to get a hotplug event just as the module is loading. That should be rare, but if it is indeed a bug, it would be nice to fix, it would be a panic for sure not to initialize VMX.

Hmm, is a cpu hotplug notifier allowed to run sleepy code? Can't imagine. So we already have a strong reason to convert kvm_lock to a raw_spinlock which obsoletes the above workaround.

Quoted 68 lines · Jan Kiszka

Quoted 60 lines · Zachary Amsden

Quoted 48 lines · Jan Kiszka

Quoted 12 lines · Zachary Amsden

Quoted 5 lines · Jan Kiszka

Code under this lock requires non-preemptibility. Ensure this also over -rt by converting it to raw spinlock.

Oh dear, I had forgotten about that. I believe kvm_lock might have the same assumption in a few places regarding clock.

I only found a problematic section in kvmclock_cpufreq_notifier. Didn't see this during my tests as I have CPUFREQ disabled in my .config.

We may need something like this as converting kvm_lock would likely be overkill:

arch/x86/kvm/x86.c +4 -2

index 36f54fb..971ee0d 100644
@@ -4530,7 +4530,7 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
   	struct cpufreq_freqs *freq = data;
   	struct kvm *kvm;
   	struct kvm_vcpu *vcpu;
-	int i, send_ipi = 0;
+	int i, me, send_ipi = 0;
 
   	/*
   	 * We allow guests to temporarily run on slowing clocks,

@@ -4583,9 +4583,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va
   		kvm_for_each_vcpu(i, vcpu, kvm) {
   			if (vcpu->cpu != freq->cpu)
   				continue;
+			me = get_cpu();
   			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
-			if (vcpu->cpu != smp_processor_id())
+			if (vcpu->cpu != me)
   				send_ipi = 1;
+			put_cpu();
   		}
   	}
   	spin_unlock(&kvm_lock);

Quoted 4 lines

Jan

That looks like a good solution, and I do believe that is the only place the lock is used in that fashion - please add a comment though in the giant comment block above that preemption protection is needed for RT. Also, gcc should catch this, but moving the me variable into the kvm_for_each_vcpu loop should allow for better register allocation.

Quoted 6 lines

The only other thing I can think of is that RT lock preemption may break some of the CPU initialization semantics enforced by kvm_lock if you happen to get a hotplug event just as the module is loading. That should be rare, but if it is indeed a bug, it would be nice to fix, it would be a panic for sure not to initialize VMX.

Quoted 4 lines

Hmm, is a cpu hotplug notifier allowed to run sleepy code? Can't imagine. So we already have a strong reason to convert kvm_lock to a raw_spinlock which obsoletes the above workaround.

I don't know as it is allowed to sleep, it doesn't call any sleeping functions to my knowledge. What worries me in the RT case is that the spinlock acquired for hardware_enable might be preempted and run on another CPU, which obviously isn't what you want.

Quoted 5 lines

I don't know as it is allowed to sleep, it doesn't call any sleeping functions to my knowledge. What worries me in the RT case is that the spinlock acquired for hardware_enable might be preempted and run on another CPU, which obviously isn't what you want.

I see now, there are calls to raw_smp_processor_id.

I think it's best to make this a raw lock. At this chance, some read-only users of vm_list should be rcu'ified. Will have a look.

Quoted 6 lines · Jan Kiszka

Quoted 5 lines

I don't know as it is allowed to sleep, it doesn't call any sleeping functions to my knowledge. What worries me in the RT case is that the spinlock acquired for hardware_enable might be preempted and run on another CPU, which obviously isn't what you want.

Quoted 4 lines

I see now, there are calls to raw_smp_processor_id.

I think it's best to make this a raw lock. At this chance, some read-only users of vm_list should be rcu'ified. Will have a look.

vm_list is rarely used, for either read or write. I don't see the need to rcu it.

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

Quoted 5 lines

I don't know as it is allowed to sleep, it doesn't call any sleeping functions to my knowledge. What worries me in the RT case is that the spinlock acquired for hardware_enable might be preempted and run on another CPU, which obviously isn't what you want.

I see now, there are calls to raw_smp_processor_id.

I think it's best to make this a raw lock. At this chance, some read-only users of vm_list should be rcu'ified. Will have a look.

Quoted 2 lines

vm_list is rarely used, for either read or write. I don't see the need to rcu it.

Avoid that code under this lock expands the preempt-disabled period, specifically under -rt, and specifically as the number of objects over which we loop is user-defined.

Quoted 16 lines · Jan Kiszka

Quoted 14 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

Quoted 5 lines

I don't know as it is allowed to sleep, it doesn't call any sleeping functions to my knowledge. What worries me in the RT case is that the spinlock acquired for hardware_enable might be preempted and run on another CPU, which obviously isn't what you want.

I see now, there are calls to raw_smp_processor_id.

I think it's best to make this a raw lock. At this chance, some read-only users of vm_list should be rcu'ified. Will have a look.

vm_list is rarely used, for either read or write. I don't see the need to rcu it.

Quoted 3 lines

Avoid that code under this lock expands the preempt-disabled period, specifically under -rt, and specifically as the number of objects over which we loop is user-defined.

Good point; even under non-rt.

(well, actually, cpufreq_notifier and kvm_arch_hardware_enable are already non preemptible, and the stats code should just go away?)

Quoted 3 lines

(well, actually, cpufreq_notifier and kvm_arch_hardware_enable are already non preemptible, and the stats code should just go away?)

The stats code is trivial to convert, so it doesn't matter.

But what about mmu_shrink and its list_move_tail? How is this synchronized against kvm_destroy_vm - already today?

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

(well, actually, cpufreq_notifier and kvm_arch_hardware_enable are already non preemptible, and the stats code should just go away?)

The stats code is trivial to convert, so it doesn't matter.

Removal is easier.

Quoted 3 lines

But what about mmu_shrink and its list_move_tail? How is this synchronized against kvm_destroy_vm - already today?

kvm_destroy_vm() takes kvm_lock. If a vm is destroyed before mmu_shrink(), mmu_shrink() will never see it. If we reach mmu_shrink() before kvm_destroy_vm(), the latter will wait until mmu_shrink() is done.

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

(well, actually, cpufreq_notifier and kvm_arch_hardware_enable are already non preemptible, and the stats code should just go away?)

The stats code is trivial to convert, so it doesn't matter.

Removal is easier.

Is that stat interface no longer used?

Quoted 8 lines

Quoted 3 lines

But what about mmu_shrink and its list_move_tail? How is this synchronized against kvm_destroy_vm - already today?

kvm_destroy_vm() takes kvm_lock. If a vm is destroyed before mmu_shrink(), mmu_shrink() will never see it. If we reach mmu_shrink() before kvm_destroy_vm(), the latter will wait until mmu_shrink() is done.

Ah, I was confused. Would require some more logic if we wanted to make the loop lock-less, though.

Quoted 8 lines · Jan Kiszka

Quoted 7 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

(well, actually, cpufreq_notifier and kvm_arch_hardware_enable are already non preemptible, and the stats code should just go away?)

The stats code is trivial to convert, so it doesn't matter.

Quoted 3 lines

Quoted 1 line

Removal is easier.

Is that stat interface no longer used?

It's there for compatibility. I'm itching to remove it. See qemu-kvm.git/kvm/kvm_stat for the only known user, and for the replacement via tracepoints.

Tracepoints have marginally lower overhead when disabled, and somewhat higher overhead when enabled. A disadvantage of tracepoints is that it is harder to associate an event with a vm when that event is triggered by a workqueue, but I don't think it matters in practice (kvm_stat doesn't even provide a per-vm breakdown).

Quoted 6 lines

Tracepoints have marginally lower overhead when disabled, and somewhat higher overhead when enabled. A disadvantage of tracepoints is that it is harder to associate an event with a vm when that event is triggered by a workqueue, but I don't think it matters in practice (kvm_stat doesn't even provide a per-vm breakdown).

What about using the perf infrastructure for this? Besides that perf can reuse tracepoints, maybe there is even a more efficient way of added new stat sources.

Code under this lock requires non-preemptibility. Ensure this also over -rt by converting it to raw spinlock.