diff mbox

[v2] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value

Message ID 20170512141322.GC2173@potion (mailing list archive)
State New, archived
Headers show

Commit Message

Radim Krčmář May 12, 2017, 2:13 p.m. UTC 
2017-05-11 12:39-0300, Marcelo Tosatti:
> On Wed, May 10, 2017 at 08:04:31PM +0200, Radim Krčmář wrote:
> > 2017-05-03 10:43-0300, Marcelo Tosatti:
> > and the important fix for kvm master clock is the move of
> > kvm_gen_update_masterclock() before we read the time.

> > The rest is just a minor optimization that also ignores time since
> > master_kernel_ns() and therefore pins user_ns.clock to a slightly
> > earlier time.
> > 
> > But all attention was given to the "minor optimization" -- have I missed
> > something about the direct use of ka->master_kernel_ns?
> 
> I haven't attempted to optimize anything. Not sure what you mean.

I mean, why doesn't the patch look like this?


> >  disabled, but call to kvm_gen_update_masterclock() would enable it)
> >    The master clock will be updates with the kernel boot clock when a
> >    VCPU runs.  This means that master clock and kernel boot clock will
> >    begin diverging at a later point, but the initial offset is the same.
> >    Userspace can only tell the difference by calling KVM_GET_CLOCK
> >    afterwards and seeing the stable bit unset.
> >    Guest is mostly unaffected -- the inaccuracy from calling
> >    KVM_SET_CLOCK is much bigger than accumulation of a slightly
> >    different frequency will in few jiffies.
> > 
> > Do you see a reason to use kvm_gen_update_masterclock()?
> > 
> > I think that the code could be just:
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 464da936c53d..f024216a858d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4177,7 +4177,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >  		r = 0;
> >  		now_ns = get_kvmclock_ns(kvm);
> >  		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> > -		kvm_gen_update_masterclock(kvm);
> > +		kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
> 
> No, this is wrong. kvm_gen_update_masterclock() must happen before
> the assignment of kvm->arch.kvmclock_offset and get_kvmclock_ns().
> 
> (so that kvm_gen_update_masterclock sets ka->use_master_clock to true 
> and get_kvmclock_ns() enters this case:
> 
>         hv_clock.tsc_timestamp = ka->master_cycle_now;
>         hv_clock.system_time = ka->master_kernel_ns +
> ka->kvmclock_offset;
>         spin_unlock(&ka->pvclock_gtod_sync_lock);
> 
>         kvm_get_time_scale(NSEC_PER_SEC, __this_cpu_read(cpu_tsc_khz) *
> 1000LL,
>                            &hv_clock.tsc_shift,
>                            &hv_clock.tsc_to_system_mul);
>         return __pvclock_read_cycles(&hv_clock, rdtsc());
> 
> Makes sense?

That doesn't explain the need for kvm_gen_update_masterclock().
kvm_gen_update_masterclock() moves the master clock base to the current
kernel boot clock; the non-master clock case uses the same kernel boot
clock to return the current kvmclock -- the computed offset is the same
in both cases.

Updating the master clock simplifies the case by resetting possible
divergence (due to different frequencies) between master clock and
kernel boot clock, but I'd still like to know (and document) if that is
the reason.

In what test does updating the master clock make a difference?

Thanks.

Comments

Paolo Bonzini May 12, 2017, 2:36 p.m. UTC | #1 
On 12/05/2017 16:13, Radim Krčmář wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 464da936c53d..8db1d09e59d7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			goto out;
>  
>  		r = 0;
> +		kvm_gen_update_masterclock(kvm);
>  		now_ns = get_kvmclock_ns(kvm);
>  		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> -		kvm_gen_update_masterclock(kvm);
> +		kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
>  		break;
>  	}
>  	case KVM_GET_CLOCK: {

And then I think the first kvm_gen_update_masterclock should be modified
to skip sending the KVM_REQ_CLOCK_UPDATE request.

Paolo
Marcelo Tosatti May 12, 2017, 3:31 p.m. UTC | #2 
On Fri, May 12, 2017 at 04:13:23PM +0200, Radim Krčmář wrote:
> 2017-05-11 12:39-0300, Marcelo Tosatti:
> > On Wed, May 10, 2017 at 08:04:31PM +0200, Radim Krčmář wrote:
> > > 2017-05-03 10:43-0300, Marcelo Tosatti:
> > > and the important fix for kvm master clock is the move of
> > > kvm_gen_update_masterclock() before we read the time.
> 
> > > The rest is just a minor optimization that also ignores time since
> > > master_kernel_ns() and therefore pins user_ns.clock to a slightly
> > > earlier time.
> > > 
> > > But all attention was given to the "minor optimization" -- have I missed
> > > something about the direct use of ka->master_kernel_ns?
> > 
> > I haven't attempted to optimize anything. Not sure what you mean.
> 
> I mean, why doesn't the patch look like this?
d 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 464da936c53d..8db1d09e59d7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			goto out;
>  
>  		r = 0;
> +		kvm_gen_update_masterclock(kvm);
>  		now_ns = get_kvmclock_ns(kvm);
>  		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> -		kvm_gen_update_masterclock(kvm);
> +		kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
>  		break;

        now_ns = ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
        kvmclock_offset += user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
        kvmclock_offset = kvmclock_offset_prev + user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)

In case of VM was just initialized before migration, kvmclock_offset_prev is -ktime_get_boot_ns()

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ka->master_kernel_ns -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))

But master_kernel_ns = ktime_get_boot_ns()     +  delta-between-vm-init-and-KVM_SET_CLOCK (AKA delta)
                    (the same one from VM init) 

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ktime_get_boot_ns() + delta + -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))

        kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - delta - grdtsc() + ka->master_cycle_now

But we don't want grdtsc() - ka->master_cycle_now in there.

Note: grdtsc() == guest read tsc.

Now with

+			kvm->arch.kvmclock_offset = user_ns.clock -
+							ka->master_kernel_ns;

What happens is that guest clock starts counting, via kernel timekeeper,
at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() -
ka->master_cycle_now in there, you are mindfully counting clock twice
(first: kernel timekeeper, second: the TSC between the (grdtsc() -
ka->master_cycle_now) in question.

+			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta

Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling
etc.

Makes sense?
Radim Krčmář May 12, 2017, 5:37 p.m. UTC | #3 
2017-05-12 12:31-0300, Marcelo Tosatti:
> On Fri, May 12, 2017 at 04:13:23PM +0200, Radim Krčmář wrote:
> > 2017-05-11 12:39-0300, Marcelo Tosatti:
> > > On Wed, May 10, 2017 at 08:04:31PM +0200, Radim Krčmář wrote:
> > > > 2017-05-03 10:43-0300, Marcelo Tosatti:
> > > > and the important fix for kvm master clock is the move of
> > > > kvm_gen_update_masterclock() before we read the time.
> > 
> > > > The rest is just a minor optimization that also ignores time since
> > > > master_kernel_ns() and therefore pins user_ns.clock to a slightly
> > > > earlier time.
> > > > 
> > > > But all attention was given to the "minor optimization" -- have I missed
> > > > something about the direct use of ka->master_kernel_ns?
> > > 
> > > I haven't attempted to optimize anything. Not sure what you mean.
> > 
> > I mean, why doesn't the patch look like this?
> d 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 464da936c53d..8db1d09e59d7 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >  			goto out;
> >  
> >  		r = 0;
> > +		kvm_gen_update_masterclock(kvm);
> >  		now_ns = get_kvmclock_ns(kvm);
> >  		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> > -		kvm_gen_update_masterclock(kvm);
> > +		kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
> >  		break;
> 
>         now_ns = ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
>         kvmclock_offset += user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
>         kvmclock_offset = kvmclock_offset_prev + user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
> 
> In case of VM was just initialized before migration, kvmclock_offset_prev is -ktime_get_boot_ns()
> 
>         kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ka->master_kernel_ns -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))
> 
> But master_kernel_ns = ktime_get_boot_ns()     +  delta-between-vm-init-and-KVM_SET_CLOCK (AKA delta)
>                     (the same one from VM init) 
> 
>         kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ktime_get_boot_ns() + delta + -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))
> 
>         kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - delta - grdtsc() + ka->master_cycle_now
> 
> But we don't want grdtsc() - ka->master_cycle_now in there.
> 
> Note: grdtsc() == guest read tsc.
> 
> Now with
> 
> +			kvm->arch.kvmclock_offset = user_ns.clock -
> +							ka->master_kernel_ns;
> 
> What happens is that guest clock starts counting, via kernel timekeeper,
> at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() -
> ka->master_cycle_now in there, you are mindfully counting clock twice
> (first: kernel timekeeper, second: the TSC between the (grdtsc() -
> ka->master_cycle_now) in question.
> 
> +			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta
> 
> Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling
> etc.
> 
> Makes sense?

Yes.  The simpler code starts the kvmclock a bit later, but both are
correct -- anything within KVM_SET_CLOCK runtime is.

If we care about accuracy, then we should let userspace provide a
(kernel timestamp, kvm timestamp) pair, so the value of kvmclock can
really be controlled.

Adding ugly optimizations to work around shortcomings of the API is
going the wrong way ...
Marcelo Tosatti May 13, 2017, 3:46 a.m. UTC | #4 
On Fri, May 12, 2017 at 07:37:12PM +0200, Radim Krčmář wrote:
> 2017-05-12 12:31-0300, Marcelo Tosatti:
> > On Fri, May 12, 2017 at 04:13:23PM +0200, Radim Krčmář wrote:
> > > 2017-05-11 12:39-0300, Marcelo Tosatti:
> > > > On Wed, May 10, 2017 at 08:04:31PM +0200, Radim Krčmář wrote:
> > > > > 2017-05-03 10:43-0300, Marcelo Tosatti:
> > > > > and the important fix for kvm master clock is the move of
> > > > > kvm_gen_update_masterclock() before we read the time.
> > > 
> > > > > The rest is just a minor optimization that also ignores time since
> > > > > master_kernel_ns() and therefore pins user_ns.clock to a slightly
> > > > > earlier time.
> > > > > 
> > > > > But all attention was given to the "minor optimization" -- have I missed
> > > > > something about the direct use of ka->master_kernel_ns?
> > > > 
> > > > I haven't attempted to optimize anything. Not sure what you mean.
> > > 
> > > I mean, why doesn't the patch look like this?
> > d 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 464da936c53d..8db1d09e59d7 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -4175,9 +4175,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
> > >  			goto out;
> > >  
> > >  		r = 0;
> > > +		kvm_gen_update_masterclock(kvm);
> > >  		now_ns = get_kvmclock_ns(kvm);
> > >  		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> > > -		kvm_gen_update_masterclock(kvm);
> > > +		kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
> > >  		break;
> > 
> >         now_ns = ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
> >         kvmclock_offset += user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
> >         kvmclock_offset = kvmclock_offset_prev + user_ns.clock - (ka->master_kernel_ns + kvmclock_offset_prev + (grdtsc() - ka->master_cycle_now)
> > 
> > In case of VM was just initialized before migration, kvmclock_offset_prev is -ktime_get_boot_ns()
> > 
> >         kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ka->master_kernel_ns -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))
> > 
> > But master_kernel_ns = ktime_get_boot_ns()     +  delta-between-vm-init-and-KVM_SET_CLOCK (AKA delta)
> >                     (the same one from VM init) 
> > 
> >         kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - (ktime_get_boot_ns() + delta + -ktime_get_boot_ns() + grdtsc() - ka->master_cycle_now))
> > 
> >         kvmclock_offset = -ktime_get_boot_ns() + user_ns.clock - delta - grdtsc() + ka->master_cycle_now
> > 
> > But we don't want grdtsc() - ka->master_cycle_now in there.
> > 
> > Note: grdtsc() == guest read tsc.
> > 
> > Now with
> > 
> > +			kvm->arch.kvmclock_offset = user_ns.clock -
> > +							ka->master_kernel_ns;
> > 
> > What happens is that guest clock starts counting, via kernel timekeeper,
> > at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() -
> > ka->master_cycle_now in there, you are mindfully counting clock twice
> > (first: kernel timekeeper, second: the TSC between the (grdtsc() -
> > ka->master_cycle_now) in question.
> > 
> > +			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta
> > 
> > Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling
> > etc.
> > 
> > Makes sense?
> 
> Yes.  The simpler code starts the kvmclock a bit later, but both are
> correct -- anything within KVM_SET_CLOCK runtime is.

No the simpler code is not correct. You count time with two clocks for a
small period of time.

KVM_SET_CLOCK means: set the guest clock to the passed value and start
counting it from there.

With the simple fix, KVM_SET_CLOCK does: set the guest clock to the
passed value, but also add the delta between kvm_get_time_and_clockread() 
and get_kvmclock_ns(). 
Which is variable, due to scheduling.
So it is just wrong.

> If we care about accuracy, then we should let userspace provide a
> (kernel timestamp, kvm timestamp) pair, so the value of kvmclock can
> really be controlled.

I suppose something else has to be done: the control of the clock, 
from whatever userspace is using to measure passage of time, 
to TSC, has to be done in kernel.

But lets see if that is really necessary when the QEMU
PTP/CLOCK_MONOTONIC delta stuff is done (working on it).

In the meantime, do you have anything against this patch? I depend on 
it for the work above.

> Adding ugly optimizations to work around shortcomings of the API is
> going the wrong way ...

What optimization you refer to?
Radim Krčmář May 15, 2017, 4:19 p.m. UTC | #5 
2017-05-13 00:46-0300, Marcelo Tosatti:
> On Fri, May 12, 2017 at 07:37:12PM +0200, Radim Krčmář wrote:
> > 2017-05-12 12:31-0300, Marcelo Tosatti:
> > > Now with
> > > 
> > > +			kvm->arch.kvmclock_offset = user_ns.clock -
> > > +							ka->master_kernel_ns;
> > > 
> > > What happens is that guest clock starts counting, via kernel timekeeper,
> > > at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() -
> > > ka->master_cycle_now in there, you are mindfully counting clock twice
> > > (first: kernel timekeeper, second: the TSC between the (grdtsc() -
> > > ka->master_cycle_now) in question.
> > > 
> > > +			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta
> > > 
> > > Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling
> > > etc.
> > > 
> > > Makes sense?
> > 
> > Yes.  The simpler code starts the kvmclock a bit later, but both are
> > correct -- anything within KVM_SET_CLOCK runtime is.
> 
> No the simpler code is not correct. You count time with two clocks for a
> small period of time.

A clock that counts kernel-nanoseconds is instantly replaced by a clock
that counts masterclock-nanoseconds, not incorrect by itself.

The simpler code will get the same kvmclock_offset as your code where
kvm_get_time_and_clockread() is called a bit later.
The distribution of resulting kvm_offsets will differ, but they must
both be correct or both incorrect, because they are already off-mark.

> KVM_SET_CLOCK means: set the guest clock to the passed value and start
> counting it from there.

Which is exactly what both versions do.

> With the simple fix, KVM_SET_CLOCK does: set the guest clock to the
> passed value, but also add the delta between kvm_get_time_and_clockread() 
> and get_kvmclock_ns(). 
> 
> Which is variable, due to scheduling.

Yes.

> So it is just wrong.

It makes the matter slightly worse by adding some execution time, but
the whole interface is "just wrong" even without that: we already have
the variability of the time between userspace's decision on
user_ns.clock value and kvm_get_time_and_clockread().

>> If we care about accuracy, then we should let userspace provide a
>> (kernel timestamp, kvm timestamp) pair, so the value of kvmclock can
>> really be controlled.
> 
> I suppose something else has to be done: the control of the clock, 
> from whatever userspace is using to measure passage of time, 
> to TSC, has to be done in kernel.

We agree, just worded it differently.

> But lets see if that is really necessary when the QEMU
> PTP/CLOCK_MONOTONIC delta stuff is done (working on it).

Right.

> In the meantime, do you have anything against this patch? I depend on 
> it for the work above.

The reasoning provided with the patch does not explain why

     * kvmclock_offset must be adjusted so that
     * user_ns.clock = master_kernel_ns + kvmclock_offset

Please explain why it "must".  I assert that it does not have to be.

If we agree that it is not necessary, then it is an optimization and I'd
like numbers to show that we are getting something that balances the
obfuscation; and why do we want it if we don't care about the better
solution (discussed above).

>                                                           I depend on 
> it for the work above.

Describing how other code couldn't work without this is great reason,
but again, please be specific -- what difference it make?

>> Adding ugly optimizations to work around shortcomings of the API is
>> going the wrong way ...
> 
> What optimization you refer to?

I refer to everything on top of the second hunk I posted.

Thanks.
Marcelo Tosatti May 15, 2017, 9:06 p.m. UTC | #6 
On Mon, May 15, 2017 at 06:19:57PM +0200, Radim Krčmář wrote:
> 2017-05-13 00:46-0300, Marcelo Tosatti:
> > On Fri, May 12, 2017 at 07:37:12PM +0200, Radim Krčmář wrote:
> > > 2017-05-12 12:31-0300, Marcelo Tosatti:
> > > > Now with
> > > > 
> > > > +			kvm->arch.kvmclock_offset = user_ns.clock -
> > > > +							ka->master_kernel_ns;
> > > > 
> > > > What happens is that guest clock starts counting, via kernel timekeeper,
> > > > at the moment kvm_get_time_and_clockread() runs. If you add grdtsc() -
> > > > ka->master_cycle_now in there, you are mindfully counting clock twice
> > > > (first: kernel timekeeper, second: the TSC between the (grdtsc() -
> > > > ka->master_cycle_now) in question.
> > > > 
> > > > +			kvm->arch.kvmclock_offset =  -ktime_get_boot_ns() +user_ns.clock -delta
> > > > 
> > > > Note that (grdtsc() - ka->master_cycle_now) is susceptible to scheduling
> > > > etc.
> > > > 
> > > > Makes sense?
> > > 
> > > Yes.  The simpler code starts the kvmclock a bit later, but both are
> > > correct -- anything within KVM_SET_CLOCK runtime is.
> > 
> > No the simpler code is not correct. You count time with two clocks for a
> > small period of time.
> 
> A clock that counts kernel-nanoseconds is instantly replaced by a clock
> that counts masterclock-nanoseconds, not incorrect by itself.
> 
> The simpler code will get the same kvmclock_offset as your code where
> kvm_get_time_and_clockread() is called a bit later.
> The distribution of resulting kvm_offsets will differ, but they must
> both be correct or both incorrect, because they are already off-mark.
> 
> > KVM_SET_CLOCK means: set the guest clock to the passed value and start
> > counting it from there.
> 
> Which is exactly what both versions do.
> 
> > With the simple fix, KVM_SET_CLOCK does: set the guest clock to the
> > passed value, but also add the delta between kvm_get_time_and_clockread() 
> > and get_kvmclock_ns(). 
> > 
> > Which is variable, due to scheduling.
> 
> Yes.
> 
> > So it is just wrong.
> 
> It makes the matter slightly worse by adding some execution time, but
> the whole interface is "just wrong" even without that: we already have
> the variability of the time between userspace's decision on
> user_ns.clock value and kvm_get_time_and_clockread().
> 
> >> If we care about accuracy, then we should let userspace provide a
> >> (kernel timestamp, kvm timestamp) pair, so the value of kvmclock can
> >> really be controlled.
> > 
> > I suppose something else has to be done: the control of the clock, 
> > from whatever userspace is using to measure passage of time, 
> > to TSC, has to be done in kernel.
> 
> We agree, just worded it differently.
> 
> > But lets see if that is really necessary when the QEMU
> > PTP/CLOCK_MONOTONIC delta stuff is done (working on it).
> 
> Right.
> 
> > In the meantime, do you have anything against this patch? I depend on 
> > it for the work above.
> 
> The reasoning provided with the patch does not explain why
> 
>      * kvmclock_offset must be adjusted so that
>      * user_ns.clock = master_kernel_ns + kvmclock_offset
> 
> Please explain why it "must".  I assert that it does not have to be.
> 
> If we agree that it is not necessary, then it is an optimization and I'd
> like numbers to show that we are getting something that balances the
> obfuscation; and why do we want it if we don't care about the better
> solution (discussed above).
> 
> >                                                           I depend on 
> > it for the work above.
> 
> Describing how other code couldn't work without this is great reason,
> but again, please be specific -- what difference it make?
> 
> >> Adding ugly optimizations to work around shortcomings of the API is
> >> going the wrong way ...
> > 
> > What optimization you refer to?
> 
> I refer to everything on top of the second hunk I posted.
> 
> Thanks.

Actually you are right, your patch is fine because the 
length of time between kvm_get_time_and_clockread()
and get_kvmclock_ns(kvm) is compensated by

- grdtsc() + ka->master_cycle_now = 
- ( +grdtsc() - ka->master_cycle_now)

Which is the length of time between kvm_get_time_and_clockread() 
and get_kvmclock_ns(kvm).

Its much cleaner indeed. Can you please apply it? Thanks.
diff mbox

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 464da936c53d..8db1d09e59d7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4175,9 +4175,10 @@  long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 
 		r = 0;
+		kvm_gen_update_masterclock(kvm);
 		now_ns = get_kvmclock_ns(kvm);
 		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
-		kvm_gen_update_masterclock(kvm);
+		kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
 		break;
 	}
 	case KVM_GET_CLOCK: {