KVM: x86: update masterclock when kvmclock_offset is calculated

The offset to add to the hosts monotonic time, kvmclock_offset, is calculated against the monotonic time at KVM_SET_CLOCK ioctl time.

Request a master clock update at this time, to reduce a potentially unbounded difference between the values of the masterclock and the clock value used to calculate kvmclock_offset.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
===================================================================

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:

Quoted 10 lines

The offset to add to the hosts monotonic time, kvmclock_offset, is calculated against the monotonic time at KVM_SET_CLOCK ioctl time.

Request a master clock update at this time, to reduce a potentially unbounded difference between the values of the masterclock and the clock value used to calculate kvmclock_offset.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 3812

Quoted 12 lines

Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
===================================================================

arch/x86/kvm/x86.c +1 -0

@@ -3806,6 +3806,7 @@ long kvm_arch_vm_ioctl(struct file *filp
 		delta = user_ns.clock - now_ns;
 		local_irq_enable();
 		kvm->arch.kvmclock_offset = delta;
+		kvm_gen_update_masterclock(kvm);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Reviewed-by Paolo Bonzini <[email protected]>

Il 22/08/2013 19:05, Paolo Bonzini ha scritto:

Quoted 11 lines

Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:

Quoted 10 lines

The offset to add to the hosts monotonic time, kvmclock_offset, is calculated against the monotonic time at KVM_SET_CLOCK ioctl time.

Request a master clock update at this time, to reduce a potentially unbounded difference between the values of the masterclock and the clock value used to calculate kvmclock_offset.

Signed-off-by Marcelo Tosatti <[email protected]>

Quoted 14 lines

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 3812

Quoted 12 lines

Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
===================================================================

arch/x86/kvm/x86.c +1 -0

@@ -3806,6 +3806,7 @@ long kvm_arch_vm_ioctl(struct file *filp
 		delta = user_ns.clock - now_ns;
 		local_irq_enable();
 		kvm->arch.kvmclock_offset = delta;
+		kvm_gen_update_masterclock(kvm);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Reviewed-by Paolo Bonzini <[email protected]>

Actually, what commit is this patch based on? You are calling the function at line 3799, but it is only declared at line 5786, and this causes:

/root/kvm-kmod/x86/x86.c: In function ‘kvm_arch_vm_ioctl’:
/root/kvm-kmod/x86/x86.c:3799:3: error: implicit declaration of function ‘kvm_gen_update_masterclock’ /root/kvm-kmod/x86/x86.c: At top level:
/root/kvm-kmod/x86/x86.c:5786:13: warning: conflicting types for ‘kvm_gen_update_masterclock’ /root/kvm-kmod/x86/x86.c:5786:13: error: static declaration of ‘kvm_gen_update_masterclock’ follows non-static declaration /root/kvm-kmod/x86/x86.c:3799:3: note: previous implicit declaration of ‘kvm_gen_update_masterclock’ was here

Paolo

Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:

Quoted 15 lines

The offset to add to the hosts monotonic time, kvmclock_offset, is calculated against the monotonic time at KVM_SET_CLOCK ioctl time.

Request a master clock update at this time, to reduce a potentially unbounded difference between the values of the masterclock and the clock value used to calculate kvmclock_offset.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
===================================================================

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 3812

Quoted 7 lines

arch/x86/kvm/x86.c +1 -0

@@ -3806,6 +3806,7 @@ long kvm_arch_vm_ioctl(struct file *filp
 		delta = user_ns.clock - now_ns;
 		local_irq_enable();
 		kvm->arch.kvmclock_offset = delta;
+		kvm_gen_update_masterclock(kvm);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Reviewed-by Paolo Bonzini <[email protected]>

While reviewing this patch, which BTW looks good, I noticed the handling of KVM_REQ_MCLOCK_INPROGRESS, the dummy request that is never processed and is only used to block guest entry.

It seems to me that this bit is not necessary. After KVM_REQ_CLOCK_UPDATE is issued, no guest entries will happen because kvm_guest_time_update will try to take the pvclock_gtod_sync_lock, currently taken by kvm_gen_update_masterclock.

Thus, you do not need the dummy request. You can simply issue KVM_REQ_CLOCK_UPDATE before calling pvclock_update_vm_gtod_copy (with the side effect of exiting VCPUs). VCPUs will stall in kvm_guest_time_update until pvclock_gtod_sync_lock is released by kvm_gen_update_masterclock. What do you think?

On top of this, optionally the spinlock could become an rw_semaphore so that clock updates for different VCPUs will not be serialized. The effect is probably not visible, though.

Quoted 26 lines

Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:

Quoted 15 lines

The offset to add to the hosts monotonic time, kvmclock_offset, is calculated against the monotonic time at KVM_SET_CLOCK ioctl time.

Request a master clock update at this time, to reduce a potentially unbounded difference between the values of the masterclock and the clock value used to calculate kvmclock_offset.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
===================================================================

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 3812

Quoted 7 lines

arch/x86/kvm/x86.c +1 -0

@@ -3806,6 +3806,7 @@ long kvm_arch_vm_ioctl(struct file *filp
 		delta = user_ns.clock - now_ns;
 		local_irq_enable();
 		kvm->arch.kvmclock_offset = delta;
+		kvm_gen_update_masterclock(kvm);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Reviewed-by Paolo Bonzini <[email protected]>

Quoted 8 lines

While reviewing this patch, which BTW looks good, I noticed the handling of KVM_REQ_MCLOCK_INPROGRESS, the dummy request that is never processed and is only used to block guest entry.

It seems to me that this bit is not necessary. After KVM_REQ_CLOCK_UPDATE is issued, no guest entries will happen because kvm_guest_time_update will try to take the pvclock_gtod_sync_lock, currently taken by kvm_gen_update_masterclock.

Not entirely clear, to cancel guest entry the bit is necessary:

        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }

Quoted 5 lines

Thus, you do not need the dummy request. You can simply issue KVM_REQ_CLOCK_UPDATE before calling pvclock_update_vm_gtod_copy (with the side effect of exiting VCPUs). VCPUs will stall in kvm_guest_time_update until pvclock_gtod_sync_lock is released by kvm_gen_update_masterclock. What do you think?

Not sure its safe. Can you describe the safety of your proposal in more detail ?

Quoted 3 lines

On top of this, optionally the spinlock could become an rw_semaphore so that clock updates for different VCPUs will not be serialized. The effect is probably not visible, though.

Still not clear of the benefits, but this area certainly welcomes performance improvements (the global kick is one thing we discussed and that should be improved).

The offset to add to the hosts monotonic time, kvmclock_offset, is calculated against the monotonic time at KVM_SET_CLOCK ioctl time.

Request a master clock update at this time, to reduce a potentially unbounded difference between the values of the masterclock and the clock value used to calculate kvmclock_offset.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +24 -16

index d21bce5..0a93354 100644
@@ -1457,6 +1457,29 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 #endif
 }
 
+static void kvm_gen_update_masterclock(struct kvm *kvm)
+{
+#ifdef CONFIG_X86_64
+	int i;
+	struct kvm_vcpu *vcpu;
+	struct kvm_arch *ka = &kvm->arch;
+
+	spin_lock(&ka->pvclock_gtod_sync_lock);
+	kvm_make_mclock_inprogress_request(kvm);
+	/* no guest entries from this point */
+	pvclock_update_vm_gtod_copy(kvm);
+
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		set_bit(KVM_REQ_CLOCK_UPDATE, &vcpu->requests);
+
+	/* guest entries allowed */
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
+
+	spin_unlock(&ka->pvclock_gtod_sync_lock);
+#endif
+}
+
 static int kvm_guest_time_update(struct kvm_vcpu *v)
 {
 	unsigned long flags, this_tsc_khz;

@@ -3806,6 +3829,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		delta = user_ns.clock - now_ns;
 		local_irq_enable();
 		kvm->arch.kvmclock_offset = delta;
+		kvm_gen_update_masterclock(kvm);
 		break;
 	}
 	case KVM_GET_CLOCK: {

@@ -5689,29 +5713,6 @@ static void process_nmi(struct kvm_vcpu *vcpu)
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 }
 
-static void kvm_gen_update_masterclock(struct kvm *kvm)
-{
-#ifdef CONFIG_X86_64
-	int i;
-	struct kvm_vcpu *vcpu;
-	struct kvm_arch *ka = &kvm->arch;
-
-	spin_lock(&ka->pvclock_gtod_sync_lock);
-	kvm_make_mclock_inprogress_request(kvm);
-	/* no guest entries from this point */
-	pvclock_update_vm_gtod_copy(kvm);
-
-	kvm_for_each_vcpu(i, vcpu, kvm)
-		set_bit(KVM_REQ_CLOCK_UPDATE, &vcpu->requests);
-
-	/* guest entries allowed */

Il 28/08/2013 04:52, Marcelo Tosatti ha scritto:

Quoted 38 lines · Paolo Bonzini

Quoted 26 lines

Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:

Quoted 15 lines

The offset to add to the hosts monotonic time, kvmclock_offset, is calculated against the monotonic time at KVM_SET_CLOCK ioctl time.

Request a master clock update at this time, to reduce a potentially unbounded difference between the values of the masterclock and the clock value used to calculate kvmclock_offset.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
===================================================================

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp · line 3812

Quoted 7 lines

arch/x86/kvm/x86.c +1 -0

@@ -3806,6 +3806,7 @@ long kvm_arch_vm_ioctl(struct file *filp
 		delta = user_ns.clock - now_ns;
 		local_irq_enable();
 		kvm->arch.kvmclock_offset = delta;
+		kvm_gen_update_masterclock(kvm);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Reviewed-by Paolo Bonzini <[email protected]>

Quoted 8 lines

While reviewing this patch, which BTW looks good, I noticed the handling of KVM_REQ_MCLOCK_INPROGRESS, the dummy request that is never processed and is only used to block guest entry.

It seems to me that this bit is not necessary. After KVM_REQ_CLOCK_UPDATE is issued, no guest entries will happen because kvm_guest_time_update will try to take the pvclock_gtod_sync_lock, currently taken by kvm_gen_update_masterclock.

Not entirely clear, to cancel guest entry the bit is necessary:

Quoted 9 lines

        if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
            || need_resched() || signal_pending(current)) {
                vcpu->mode = OUTSIDE_GUEST_MODE;
                smp_wmb();
                local_irq_enable();
                preempt_enable();
                r = 1;
                goto cancel_injection;
        }

Yes, KVM_REQ_CLOCK_UPDATE is enough to cancel guest entries too. See code below.

Quoted 8 lines

Quoted 5 lines

Thus, you do not need the dummy request. You can simply issue KVM_REQ_CLOCK_UPDATE before calling pvclock_update_vm_gtod_copy (with the side effect of exiting VCPUs). VCPUs will stall in kvm_guest_time_update until pvclock_gtod_sync_lock is released by kvm_gen_update_masterclock. What do you think?

Not sure its safe. Can you describe the safety of your proposal in more detail ?

Here is the code I was thinking of:

	spin_lock(&ka->pvclock_gtod_sync_lock);
	make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);

	/*
	 * No guest entries from this point: VCPUs will be spinning
	 * on pvclock_gtod_sync_lock in kvm_guest_time_update.
	 */
	pvclock_update_vm_gtod_copy(kvm);

	/*
	 * Let kvm_guest_time_update continue: entering the guest
	 * is now allowed too.
	 */
	spin_unlock(&ka->pvclock_gtod_sync_lock);

KVM_REQ_CLOCK_UPDATE is used to cancel guest entry and execute kvm_guest_time_update. But kvm_guest_time_update will spin on pvclock_gtod_sync_lock until pvclock_update_vm_gtod_copy exits and kvm_gen_update_masterclock releases the spinlock.

Quoted 7 lines

Quoted 3 lines

On top of this, optionally the spinlock could become an rw_semaphore so that clock updates for different VCPUs will not be serialized. The effect is probably not visible, though.

Still not clear of the benefits, but this area certainly welcomes performance improvements (the global kick is one thing we discussed and that should be improved).

This unfortunately does not eliminate the global kick, so there is likely no performance improvement yet. It simplifies the logic a bit though.

The change I suggested above is to make pvclock_gtod_sync_lock an rwsem or, probably better, a seqlock.
VCPUs reading ka->use_master_clock,
ka->master_cycle_now, ka->master_kernel_ns can then run concurrently, with no locked operations in kvm_guest_time_update.

Quoted 17 lines

Here is the code I was thinking of:

	spin_lock(&ka->pvclock_gtod_sync_lock);
	make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);

	/*
	 * No guest entries from this point: VCPUs will be spinning
	 * on pvclock_gtod_sync_lock in kvm_guest_time_update.
	 */
	pvclock_update_vm_gtod_copy(kvm);

	/*
	 * Let kvm_guest_time_update continue: entering the guest
	 * is now allowed too.
	 */
	spin_unlock(&ka->pvclock_gtod_sync_lock);

Quoted 4 lines

KVM_REQ_CLOCK_UPDATE is used to cancel guest entry and execute kvm_guest_time_update. But kvm_guest_time_update will spin on pvclock_gtod_sync_lock until pvclock_update_vm_gtod_copy exits and kvm_gen_update_masterclock releases the spinlock.

Not safe because there are places which set KVM_REQ_CLOCK_UPDATE without kicking target vcpu out of guest mode. Unless you use a modified make_all_cpus_request.

The point of REQ_MCLOCK_INPROGRESS request is to guarantee that the following is not possible:

- 2 vcpus in guest mode with per-vcpu kvmclock areas with different {system_timestamp, tsc_offset} values.

To achieve that:

- Kick all vcpus out of guest mode (via a request bit that can't be cleared).
- Update the {system_timestamp, tsc_offset} values.
- Clear the request bit.

Il 03/09/2013 05:03, Marcelo Tosatti ha scritto:

Quoted 22 lines

Quoted 21 lines

Here is the code I was thinking of:

	spin_lock(&ka->pvclock_gtod_sync_lock);
	make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);

	/*
	 * No guest entries from this point: VCPUs will be spinning
	 * on pvclock_gtod_sync_lock in kvm_guest_time_update.
	 */
	pvclock_update_vm_gtod_copy(kvm);

	/*
	 * Let kvm_guest_time_update continue: entering the guest
	 * is now allowed too.
	 */
	spin_unlock(&ka->pvclock_gtod_sync_lock);

KVM_REQ_CLOCK_UPDATE is used to cancel guest entry and execute kvm_guest_time_update. But kvm_guest_time_update will spin on pvclock_gtod_sync_lock until pvclock_update_vm_gtod_copy exits and kvm_gen_update_masterclock releases the spinlock.

Quoted 3 lines

Not safe because there are places which set KVM_REQ_CLOCK_UPDATE without kicking target vcpu out of guest mode. Unless you use a modified make_all_cpus_request.

make_all_cpus_request does force an exit, even if the bit is already set in vcpus->requests. But maybe here is where I'm missing something.

Quoted 5 lines

The point of REQ_MCLOCK_INPROGRESS request is to guarantee that the following is not possible:

- 2 vcpus in guest mode with per-vcpu kvmclock areas with different {system_timestamp, tsc_offset} values.

Understood.

Quoted 6 lines

To achieve that:

- Kick all vcpus out of guest mode (via a request bit that can't be cleared).
- Update the {system_timestamp, tsc_offset} values.
- Clear the request bit.

After make_all_cpus_request, all VCPUs will be out of guest mode, and the request bit will not be cleared until pvclock_gtod_sync_lock is released.

Paolo

Quoted 15 lines

Quoted 5 lines

The point of REQ_MCLOCK_INPROGRESS request is to guarantee that the following is not possible:

- 2 vcpus in guest mode with per-vcpu kvmclock areas with different {system_timestamp, tsc_offset} values.

Understood.

Quoted 6 lines

To achieve that:

- Kick all vcpus out of guest mode (via a request bit that can't be cleared).
- Update the {system_timestamp, tsc_offset} values.
- Clear the request bit.

Quoted 3 lines

After make_all_cpus_request, all VCPUs will be out of guest mode, and the request bit will not be cleared until pvclock_gtod_sync_lock is released.

It seems more obfuscated to rely on an implicit pvclock_gtod_sync_lock blocking than an explicit request bit that can't be cleared, but perhaps thats personal opinion.

OTOH, you can also argue that the request bit abuses the vcpu->requests request mechanism, because there is not a request in fact (it only blocks guest entry).

If you have reasons to update, and you are sure its safe, feel free to modify it.