unify remote request and kvm_vcpu_kick IPI mechanism

Split KVM_REQ_KICKED in two bits: KVM_VCPU_KICKED, to indicate whether a vcpu has been IPI'ed, and KVM_VCPU_GUEST_MODE, in a separate vcpu_state variable.

Unify remote requests with kvm_vcpu_kick.

Synchronous requests wait on KVM_VCPU_GUEST_MODE, via wait_on_bit/wake_up_bit.

virt/kvm/kvm_main.c +27 -3

+}
+
+void kvm_vcpu_ipi(struct kvm_vcpu *vcpu)
+{
+	int me;
+	int cpu = vcpu->cpu;
 
  	me = get_cpu();
-	if (cpu != me&&  (unsigned)cpu<  nr_cpu_ids&&  cpu_online(cpu))
-		if (!test_and_set_bit(KVM_REQ_KICK,&vcpu->requests))
-			smp_send_reschedule(cpu);
+	if (cpu != me&&  (unsigned)cpu<  nr_cpu_ids&&  cpu_online(cpu)) {
+		if (test_bit(KVM_VCPU_GUEST_MODE,&vcpu->vcpu_state)) {
+			if (!test_and_set_bit(KVM_VCPU_KICKED,
+					&vcpu->vcpu_state))
+				smp_send_reschedule(cpu);
+		}
+	}
  	put_cpu();
  }

@@ -168,6 +176,30 @@ static bool make_all_cpus_request(struct
  	return called;
  }
 
+static int kvm_req_wait(void *unused)
+{
+	cpu_relax();
+	return 0;
+}
+
+static void kvm_vcpu_request(struct kvm_vcpu *vcpu, unsigned int req)
+{
+	set_bit(req,&vcpu->requests);
+	barrier();
+	kvm_vcpu_ipi(vcpu);
+	wait_on_bit(&vcpu->vcpu_state, KVM_VCPU_GUEST_MODE, kvm_req_wait,
+		    TASK_UNINTERRUPTIBLE);
+}

Quoted 9 lines · Marcelo Tosatti

Split KVM_REQ_KICKED in two bits: KVM_VCPU_KICKED, to indicate whether a vcpu has been IPI'ed, and KVM_VCPU_GUEST_MODE, in a separate vcpu_state variable.

Unify remote requests with kvm_vcpu_kick.

Synchronous requests wait on KVM_VCPU_GUEST_MODE, via wait_on_bit/wake_up_bit.

I did miss guest_mode.

include/linux/kvm_host.h struct kvm_vcpu { · line 89

Quoted 2 lines

include/linux/kvm_host.h +1 -0

+	unsigned long vcpu_state;

Why not bool guest_mode? Saves two atomics per exit.

Quoted 46 lines · Marcelo Tosatti

virt/kvm/kvm_main.c +34 -3

+}
+
+void kvm_vcpu_ipi(struct kvm_vcpu *vcpu)
+{
+	int me;
+	int cpu = vcpu->cpu;
 
  	me = get_cpu();
-	if (cpu != me&&  (unsigned)cpu<  nr_cpu_ids&&  cpu_online(cpu))
-		if (!test_and_set_bit(KVM_REQ_KICK,&vcpu->requests))
-			smp_send_reschedule(cpu);
+	if (cpu != me&&  (unsigned)cpu<  nr_cpu_ids&&  cpu_online(cpu)) {
+		if (test_bit(KVM_VCPU_GUEST_MODE,&vcpu->vcpu_state)) {
+			if (!test_and_set_bit(KVM_VCPU_KICKED,
+					&vcpu->vcpu_state))
+				smp_send_reschedule(cpu);
+		}
+	}
  	put_cpu();
  }

@@ -168,6 +176,30 @@ static bool make_all_cpus_request(struct
  	return called;
  }
 
+static int kvm_req_wait(void *unused)
+{
+	cpu_relax();
+	return 0;
+}
+
+static void kvm_vcpu_request(struct kvm_vcpu *vcpu, unsigned int req)
+{
+	set_bit(req,&vcpu->requests);
+	barrier();
+	kvm_vcpu_ipi(vcpu);
+	wait_on_bit(&vcpu->vcpu_state, KVM_VCPU_GUEST_MODE, kvm_req_wait,
+		    TASK_UNINTERRUPTIBLE);
+}
+
+static void kvm_vcpus_request(struct kvm *kvm, unsigned int req)
+{
+	int i;
+	struct kvm_vcpu *vcpu;
+
+	kvm_for_each_vcpu(i, vcpu, kvm)

virt/kvm/kvm_main.c static bool make_all_cpus_request(struct

Quoted 3 lines

virt/kvm/kvm_main.c +2 -0

+		kvm_vcpu_request(vcpu, req);
+}
 

Gleb notes there are two problems here: instead of using a multicast IPI, you're sending multiple unicast IPIs. Second, you're serializing the waiting. It would be better to batch-send the IPIs, then batch-wait for results.

Quoted 2 lines

Why not bool guest_mode? Saves two atomics per exit.

It must be atomic since GUEST_MODE / VCPU_KICKED bits are manipulated by multiple CPU's.

Quoted 5 lines · Marcelo Tosatti

Quoted 2 lines

Why not bool guest_mode? Saves two atomics per exit.

It must be atomic since GUEST_MODE / VCPU_KICKED bits are manipulated by multiple CPU's.

bools are atomic.

perf report shows heavy overhead from down/up of slots_lock.

Attempted to remove slots_lock by having vcpus stop on a synchronization point, but this introduced further complexity (a vcpu can be scheduled out before reaching the synchronization point, and can sched back in at points which are slots_lock protected, etc).

This patch changes vcpu_enter_guest to conditionally release/acquire slots_lock in case a vcpu state bit is set.

vmexit performance improves by 5-10% on UP guest.

Quoted 7 lines · Marcelo Tosatti

perf report shows heavy overhead from down/up of slots_lock.

Attempted to remove slots_lock by having vcpus stop on a synchronization point, but this introduced further complexity (a vcpu can be scheduled out before reaching the synchronization point, and can sched back in at points which are slots_lock protected, etc).

Quoted 5 lines

This patch changes vcpu_enter_guest to conditionally release/acquire slots_lock in case a vcpu state bit is set.

vmexit performance improves by 5-10% on UP guest.

Sorry, it looks pretty complex.

Have you considered using srcu? It seems to me down/up_read() can be replaced by srcu_read_lock/unlock(), and after proper conversion of memslots and io_bus to rcu_assign_pointer(), we can just add synchronize_srcu() immediately after changing stuff (of course mmu_lock still needs to be held when updating slots).

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Marcelo Tosatti

perf report shows heavy overhead from down/up of slots_lock.

Attempted to remove slots_lock by having vcpus stop on a synchronization point, but this introduced further complexity (a vcpu can be scheduled out before reaching the synchronization point, and can sched back in at points which are slots_lock protected, etc).

This patch changes vcpu_enter_guest to conditionally release/acquire slots_lock in case a vcpu state bit is set.

Quoted 5 lines

Quoted 3 lines

vmexit performance improves by 5-10% on UP guest.

Sorry, it looks pretty complex.

Why?

Quoted 6 lines

Have you considered using srcu? It seems to me down/up_read() can be replaced by srcu_read_lock/unlock(), and after proper conversion of memslots and io_bus to rcu_assign_pointer(), we can just add synchronize_srcu() immediately after changing stuff (of course mmu_lock still needs to be held when updating slots).

I don't see RCU as being suitable because in certain operations you want to stop writers (on behalf of vcpus), do something, and let them continue afterwards. The dirty log, for example. Or any operation that wants to modify lockless vcpu specific data.

Also, synchronize_srcu() is limited to preemptible sections.

io_bus could use RCU, but I think being able to stop vcpus is also a different requirement. Do you have any suggestion on how to do it in a better way?

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Marcelo Tosatti

Quoted 3 lines

Why not bool guest_mode? Saves two atomics per exit.

It must be atomic since GUEST_MODE / VCPU_KICKED bits are manipulated by multiple CPU's.

Quoted 2 lines

bools are atomic.

OK.

- VCPU_KICKED requires test_and_set. GUEST_MODE/VCPU_KICKED accesses must not be reordered.

(OK, could have GUEST_MODE in a bool even so, but its easier to read by keeping them together, at least to me).

- Its easier to cacheline align with longs rather than bools?

- From testing it seems the LOCK prefix is not heavy, as long as its cpu local (probably due to 7.1.4 Effects of a LOCK Operation on Internal Processor Caches?).

BTW,

7.1.2.2 Software Controlled Bus Locking

Software should access semaphores (shared memory used for signalling between multiple processors) using identical addresses and operand lengths. For example, if one processor accesses a semaphore using a word access, other processors should not access the semaphore using a byte access.

The bit operations use 32-bit access, but the vcpu->requests check in vcpu_enter_guest uses 64-bit access.

Is that safe?

Why do we need both, btw? Set your vcpu->requests bit, if guest_mode is true, clear it and IPI. So guest_mode=false means, we might be in guest mode but if so we're due for a kick anyway.

Quoted 14 lines · Marcelo Tosatti

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Marcelo Tosatti

perf report shows heavy overhead from down/up of slots_lock.

Attempted to remove slots_lock by having vcpus stop on a synchronization point, but this introduced further complexity (a vcpu can be scheduled out before reaching the synchronization point, and can sched back in at points which are slots_lock protected, etc).

This patch changes vcpu_enter_guest to conditionally release/acquire slots_lock in case a vcpu state bit is set.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

vmexit performance improves by 5-10% on UP guest.

Sorry, it looks pretty complex.

Why?

There's a new locking protocol in there. I prefer sticking with the existing kernel plumbing, or it gets more and more difficult knowing who protects what and in what order you can do things.

Quoted 11 lines

Quoted 6 lines

Have you considered using srcu? It seems to me down/up_read() can be replaced by srcu_read_lock/unlock(), and after proper conversion of memslots and io_bus to rcu_assign_pointer(), we can just add synchronize_srcu() immediately after changing stuff (of course mmu_lock still needs to be held when updating slots).

I don't see RCU as being suitable because in certain operations you want to stop writers (on behalf of vcpus), do something, and let them continue afterwards. The dirty log, for example. Or any operation that wants to modify lockless vcpu specific data.

kvm_flush_remote_tlbs() (which you'd call after mmu operations), will get cpus out of guest mode, and synchronize_srcu() will wait for them to drop the srcu "read lock". So it really happens naturally: do an RCU update, send some request to all vcpus, synchronize_srcu(), done.

Quoted 2 lines

Also, synchronize_srcu() is limited to preemptible sections.

Quoted 4 lines

io_bus could use RCU, but I think being able to stop vcpus is also a different requirement. Do you have any suggestion on how to do it in a better way?

We don't need to stop vcpus, just kick them out of guest mode to let them notice the new data. SRCU does that well.

Quoted 4 lines

We don't need to stop vcpus, just kick them out of guest mode to let them notice the new data. SRCU does that well.

Two problems:

1. The removal of memslots/aliases and zapping of mmu (to remove any shadow pages with stale sp->gfn) must be atomic from the POV of the pagefault path. Otherwise something like this can happen:

fault path set_memory_region

walk_addr fetches and validates
table_gfns
delete memslot synchronize_srcu

fetch creates shadow

page with nonexistant sp->gfn

OR

mmu_alloc_roots path set_memory_region

delete memslot
root_gfn = vcpu->arch.cr3<< PAGE_SHIFT
mmu_check_root(root_gfn) synchronize_rcu
kvm_mmu_get_page()

kvm_mmu_zap_all

Accesses between kvm_mmu_get_page and kvm_mmu_zap_all window can see shadow pages with stale gfn.

But, if you still think its worthwhile to use RCU, at least handling gfn_to_memslot / unalias_gfn errors _and_ adding mmu_notifier_retry invalidation to set_memory_region path will be necessary (so that gfn_to_pfn validation, in the fault path, is discarded in case of memslot/alias update).

2. Another complication is that memslot creation and kvm_iommu_map_pages are not atomic.

create memslot
synchronize_srcu
<----- vcpu grabs gfn reference without
iommu mapping.
kvm_iommu_map_pages

Which can be solved by changing kvm_iommu_map_pages (and new gfn_to_pfn helper) to use base_gfn,npages,hva information from somewhere else other than visible kvm->memslots (so that when the slot becomes visible its already iommu mapped).

So it appears to me using RCU introduces more complications / subtle details than mutual exclusion here. The new request bit which the original patch introduces is limited to enabling/disabling conditional acquision of slots_lock (calling it a "new locking protocol" is unfair) to improve write acquision latency.

Quoted 4 lines · Marcelo Tosatti

Quoted 4 lines

We don't need to stop vcpus, just kick them out of guest mode to let them notice the new data. SRCU does that well.

Quoted 8 lines

Two problems:

1. The removal of memslots/aliases and zapping of mmu (to remove any shadow pages with stale sp->gfn) must be atomic from the POV of the pagefault path. Otherwise something like this can happen:

fault path set_memory_region

srcu_read_lock()

Quoted 7 lines

walk_addr fetches and validates
table_gfns
delete memslot synchronize_srcu

fetch creates shadow

srcu_read_unlock()

Quoted 2 lines

page with nonexistant sp->gfn

I think synchronize_srcu() will be deferred until the fault path is complete (and srcu_read_unlock() runs). Copying someone who knows for sure.

Quoted 4 lines

OR

mmu_alloc_roots path set_memory_region

srcu_read_lock()

Quoted 6 lines

delete memslot
root_gfn = vcpu->arch.cr3<< PAGE_SHIFT
mmu_check_root(root_gfn) synchronize_rcu
kvm_mmu_get_page()

srcu_read_unlock()

Quoted 2 lines

kvm_mmu_zap_all

Ditto, srcu_read_lock() protects us.

Quoted 8 lines

Accesses between kvm_mmu_get_page and kvm_mmu_zap_all window can see shadow pages with stale gfn.

But, if you still think its worthwhile to use RCU, at least handling gfn_to_memslot / unalias_gfn errors _and_ adding mmu_notifier_retry invalidation to set_memory_region path will be necessary (so that gfn_to_pfn validation, in the fault path, is discarded in case of memslot/alias update).

It really is worthwhile to reuse complex infrastructure instead of writing new infrastructure.

Quoted 9 lines

2. Another complication is that memslot creation and kvm_iommu_map_pages are not atomic.

create memslot
synchronize_srcu
<----- vcpu grabs gfn reference without
iommu mapping.
kvm_iommu_map_pages

Quoted 4 lines

Which can be solved by changing kvm_iommu_map_pages (and new gfn_to_pfn helper) to use base_gfn,npages,hva information from somewhere else other than visible kvm->memslots (so that when the slot becomes visible its already iommu mapped).

Yes. It can accept a memslots structure instead of deriving it from kvm->memslots. Then we do a rcu_assign_pointer() to switch the tables.

Quoted 6 lines

So it appears to me using RCU introduces more complications / subtle details than mutual exclusion here. The new request bit which the original patch introduces is limited to enabling/disabling conditional acquision of slots_lock (calling it a "new locking protocol" is unfair) to improve write acquision latency.

It's true that it is not a new locking protocol. But I feel it is worthwhile to try to use rcu for this; at least it will make it easier for newcomers (provided they understand rcu).

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 29 lines · Avi Kivity

Quoted 4 lines · Marcelo Tosatti

Quoted 4 lines

We don't need to stop vcpus, just kick them out of guest mode to let them notice the new data. SRCU does that well.

Quoted 8 lines

Two problems:

1. The removal of memslots/aliases and zapping of mmu (to remove any shadow pages with stale sp->gfn) must be atomic from the POV of the pagefault path. Otherwise something like this can happen:

fault path set_memory_region

srcu_read_lock()

Quoted 7 lines

walk_addr fetches and validates
table_gfns
delete memslot synchronize_srcu

fetch creates shadow

srcu_read_unlock()

Quoted 2 lines

page with nonexistant sp->gfn

Quoted 3 lines

I think synchronize_srcu() will be deferred until the fault path is complete (and srcu_read_unlock() runs). Copying someone who knows for sure.

Yes, synchronize_srcu() will block until srcu_read_unlock() in this scenario, assuming that the same srcu_struct is used by both.

Quoted 14 lines

Quoted 4 lines

OR

mmu_alloc_roots path set_memory_region

srcu_read_lock()

Quoted 6 lines

delete memslot
root_gfn = vcpu->arch.cr3<< PAGE_SHIFT
mmu_check_root(root_gfn) synchronize_rcu
kvm_mmu_get_page()

Quoted 6 lines

srcu_read_unlock()

Quoted 2 lines

kvm_mmu_zap_all

Ditto, srcu_read_lock() protects us.

Yep!

Quoted 9 lines

Quoted 8 lines

Accesses between kvm_mmu_get_page and kvm_mmu_zap_all window can see shadow pages with stale gfn.

But, if you still think its worthwhile to use RCU, at least handling gfn_to_memslot / unalias_gfn errors _and_ adding mmu_notifier_retry invalidation to set_memory_region path will be necessary (so that gfn_to_pfn validation, in the fault path, is discarded in case of memslot/alias update).

Quoted 2 lines

It really is worthwhile to reuse complex infrastructure instead of writing new infrastructure.

Marcelo, in your first example, is your concern that the fault path needs to detect the memslot deletion?

Or that the use of sp->gfn "leaks" out of the SRCU read-side critical section?

Thanx, Paul

Quoted 16 lines

Quoted 9 lines

2. Another complication is that memslot creation and kvm_iommu_map_pages are not atomic.

create memslot
synchronize_srcu
<----- vcpu grabs gfn reference without
iommu mapping.
kvm_iommu_map_pages

Quoted 4 lines

Which can be solved by changing kvm_iommu_map_pages (and new gfn_to_pfn helper) to use base_gfn,npages,hva information from somewhere else other than visible kvm->memslots (so that when the slot becomes visible its already iommu mapped).

Yes. It can accept a memslots structure instead of deriving it from kvm->memslots. Then we do a rcu_assign_pointer() to switch the tables.

Quoted 10 lines

Quoted 6 lines

So it appears to me using RCU introduces more complications / subtle details than mutual exclusion here. The new request bit which the original patch introduces is limited to enabling/disabling conditional acquision of slots_lock (calling it a "new locking protocol" is unfair) to improve write acquision latency.

It's true that it is not a new locking protocol. But I feel it is worthwhile to try to use rcu for this; at least it will make it easier for newcomers (provided they understand rcu).

Quoted 5 lines

Quoted 5 lines

I think synchronize_srcu() will be deferred until the fault path is complete (and srcu_read_unlock() runs). Copying someone who knows for sure.

Quoted 3 lines

Yes, synchronize_srcu() will block until srcu_read_unlock() in this scenario, assuming that the same srcu_struct is used by both.

Right it will. But this does not stop the fault path from creating shadow pages with stale sp->gfn (the only way to do that would be mutual exclusion AFAICS).

The RCU read-protected side does not stop a new memslots pointer from being assigned (with rcu_assign_pointer), does it?

Quoted 3 lines

Right it will. But this does not stop the fault path from creating shadow pages with stale sp->gfn (the only way to do that would be mutual exclusion AFAICS).

So we put the kvm_mmu_zap_pages() call as part of the synchronize_srcu() callback to take advantage of the srcu guarantees. We know that when when the callback is called all new reads see the new slots and all old readers have completed.