All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm-devel <kvm@vger.kernel.org>
Subject: Re: [PATCH v2] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value
Date: Wed, 10 May 2017 20:04:31 +0200	[thread overview]
Message-ID: <20170510180430.GA2240@potion> (raw)
In-Reply-To: <20170503134341.GB10468@amt.cnet>

2017-05-03 10:43-0300, Marcelo Tosatti:
> In the masterclock enabled case, kvmclock_offset must be adjusted so
> that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the
> value set from KVM_SET_CLOCK is the one visible at system_timestamp).

IIUC, we want to achieve

  user_ns.clock == get_kvmclock_ns(kvm)

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?

(If not, I'd rather have just the one-line move.)

---
Detailed reasoning.

kvm_gen_update_masterclock() shifts the master clock (changes its
percieved frequency, because it is reset from kernel boot clock again,
even though they are diverging), so we must not compute the
kvmclock_offset with an old master clock and apply it to a shifted one.

Using offset from ka->master_kernel_ns or get_kvmclock_ns()
("ka->master_kernel_ns + small delta") doesn't make much difference
then, because the user_ns is already an indeterminable point in the
past.  (We just assume that user_ns.clock is now, whenever that is.)

---
And a possible improvement.

Using kvm_gen_update_masterclock() seems superfluous.  There are three
major cases depending on state of the master clock:

 enabled)
   We can just match the kvmclock_offset so the following holds
     user_ns.clock == get_kvmclock_ns(kvm)
   No need to update the master clock.

 disabled)
   The kvmclock_offset is set from the kernel boot clock and that is
   already correct.

 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);
 		break;
 	}
 	case KVM_GET_CLOCK: {

Thanks.

  reply	other threads:[~2017-05-10 18:04 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-02 21:36 [PATCH] " Marcelo Tosatti
2017-05-03 13:08 ` Paolo Bonzini
2017-05-03 13:40   ` Marcelo Tosatti
2017-05-03 13:55     ` Paolo Bonzini
2017-05-03 18:24       ` Marcelo Tosatti
2017-05-03 13:43   ` [PATCH v2] " Marcelo Tosatti
2017-05-10 18:04     ` Radim Krčmář [this message]
2017-05-11 15:39       ` Marcelo Tosatti
2017-05-12 14:13         ` Radim Krčmář
2017-05-12 14:36           ` Paolo Bonzini
2017-05-12 15:31           ` Marcelo Tosatti
2017-05-12 17:37             ` Radim Krčmář
2017-05-13  3:46               ` Marcelo Tosatti
2017-05-15 16:19                 ` Radim Krčmář
2017-05-15 21:06                   ` Marcelo Tosatti

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170510180430.GA2240@potion \
    --to=rkrcmar@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=mtosatti@redhat.com \
    --cc=pbonzini@redhat.com \
    --subject='Re: [PATCH v2] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.