KVM: x86: update masterclock when kvmclock_offset is calculated

KVM: x86: update masterclock when kvmclock_offset is calculated

[Marcelo Tosatti]

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 <mtosatti@redhat.com>

Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
===================================================================
--- linux-2.6-kvmclock-fixes.orig/arch/x86/kvm/x86.c
+++ linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
@@ -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: {

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Paolo Bonzini]

Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:
> 
> 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 <mtosatti@redhat.com>
> 
> Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> ===================================================================
> --- linux-2.6-kvmclock-fixes.orig/arch/x86/kvm/x86.c
> +++ linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> @@ -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 <pbonzini@redhat.com>

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.

Paolo

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

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Paolo Bonzini]

Il 22/08/2013 19:05, Paolo Bonzini ha scritto:
> Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:
>>
>> 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 <mtosatti@redhat.com>
>>
>> Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
>> ===================================================================
>> --- linux-2.6-kvmclock-fixes.orig/arch/x86/kvm/x86.c
>> +++ linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
>> @@ -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 <pbonzini@redhat.com>

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

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Marcelo Tosatti]

On Thu, Aug 22, 2013 at 07:05:20PM +0200, Paolo Bonzini wrote:
> Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:
> > 
> > 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 <mtosatti@redhat.com>
> > 
> > Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> > ===================================================================
> > --- linux-2.6-kvmclock-fixes.orig/arch/x86/kvm/x86.c
> > +++ linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> > @@ -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 <pbonzini@redhat.com>
> 
> 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;
        }

> 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 ?

> 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).

KVM: x86: update masterclock when kvmclock_offset is calculated (v2)

[Marcelo Tosatti]

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 <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d21bce5..0a93354 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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 */
-	kvm_for_each_vcpu(i, vcpu, kvm)
-		clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
-
-	spin_unlock(&ka->pvclock_gtod_sync_lock);
-#endif
-}
-
 static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
 {
 	u64 eoi_exit_bitmap[4];

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Marcelo Tosatti]

On Fri, Aug 23, 2013 at 12:00:46PM +0200, Paolo Bonzini wrote:
> Il 22/08/2013 19:05, Paolo Bonzini ha scritto:
> > Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:
> >>
> >> 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 <mtosatti@redhat.com>
> >>
> >> Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> >> ===================================================================
> >> --- linux-2.6-kvmclock-fixes.orig/arch/x86/kvm/x86.c
> >> +++ linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> >> @@ -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 <pbonzini@redhat.com>
> 
> 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

Wrong tree, my bad, sorry Paolo (sent v2).

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Paolo Bonzini]

Il 28/08/2013 04:52, Marcelo Tosatti ha scritto:
> On Thu, Aug 22, 2013 at 07:05:20PM +0200, Paolo Bonzini wrote:
>> Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:
>>>
>>> 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 <mtosatti@redhat.com>
>>>
>>> Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
>>> ===================================================================
>>> --- linux-2.6-kvmclock-fixes.orig/arch/x86/kvm/x86.c
>>> +++ linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
>>> @@ -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 <pbonzini@redhat.com>
>>
>> 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;
>         }

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

>> 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.

>> 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.

Paolo

Re: KVM: x86: update masterclock when kvmclock_offset is calculated (v2)

[Gleb Natapov]

On Tue, Aug 27, 2013 at 11:55:29PM -0300, Marcelo Tosatti wrote:
> 
> 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 <mtosatti@redhat.com>
> 
Applied, thanks.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d21bce5..0a93354 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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 */
> -	kvm_for_each_vcpu(i, vcpu, kvm)
> -		clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
> -
> -	spin_unlock(&ka->pvclock_gtod_sync_lock);
> -#endif
> -}
> -
>  static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>  {
>  	u64 eoi_exit_bitmap[4];
> 

--
			Gleb.

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Marcelo Tosatti]

On Wed, Aug 28, 2013 at 02:37:20PM +0200, Paolo Bonzini wrote:
> Il 28/08/2013 04:52, Marcelo Tosatti ha scritto:
> > On Thu, Aug 22, 2013 at 07:05:20PM +0200, Paolo Bonzini wrote:
> >> Il 20/08/2013 20:20, Marcelo Tosatti ha scritto:
> >>>
> >>> 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 <mtosatti@redhat.com>
> >>>
> >>> Index: linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> >>> ===================================================================
> >>> --- linux-2.6-kvmclock-fixes.orig/arch/x86/kvm/x86.c
> >>> +++ linux-2.6-kvmclock-fixes/arch/x86/kvm/x86.c
> >>> @@ -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 <pbonzini@redhat.com>
> >>
> >> 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;
> >         }
> 
> Yes, KVM_REQ_CLOCK_UPDATE is enough to cancel guest entries too.  See
> code below.
> 
> >> 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.

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.

> >> 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.

Good point.

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Paolo Bonzini]

Il 03/09/2013 05:03, Marcelo Tosatti ha scritto:
> > 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.
> 
> 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.

> 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.

> 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

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Marcelo Tosatti]

On Tue, Sep 03, 2013 at 12:42:40PM +0200, Paolo Bonzini wrote:
> Il 03/09/2013 05:03, Marcelo Tosatti ha scritto:
> > > 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.
> > 
> > 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.

No, you're right. My bad.

> > 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.
> 
> > 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

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.

Re: KVM: x86: update masterclock when kvmclock_offset is calculated

[Paolo Bonzini]

Il 03/09/2013 15:56, Marcelo Tosatti ha scritto:
>>> 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.
>>
>>> 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.
> 
> 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.

Yes, this gets dangerously close to personal opinion territory. :)

> 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).

The busy-wait is a bit ugly, but otherwise it's fine.

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

No reason yet apart from removing a few lines of code, but thanks for
discussing this.  If you want to do the seqlock change for
pvclock_gtod_sync_lock, I'll be glad to review the patch.

Paolo