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

[Marcelo Tosatti <mtosatti@redhat.com>]

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