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.
next prev parent 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] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.