On Thu, Nov 17, 2016 at 05:15:13PM -0200, Marcelo Tosatti wrote:
> On Thu, Nov 17, 2016 at 04:16:09PM -0200, Eduardo Habkost wrote:
> > On Thu, Nov 17, 2016 at 03:15:35PM -0200, Marcelo Tosatti wrote:
> > > On Thu, Nov 17, 2016 at 12:11:26PM -0200, Eduardo Habkost wrote:
> > > > On Thu, Nov 17, 2016 at 11:24:13AM -0200, Marcelo Tosatti wrote:
> > > > > Check for KVM_CAP_ADJUST_CLOCK capability KVM_CLOCK_TSC_STABLE, which
> > > > > indicates that KVM_GET_CLOCK returns a value as seen by the guest at
> > > > > that moment.
> > > > >
> > > > > For new machine types, use this value rather than reading
> > > > > from guest memory.
> > > > >
> > > > > This reduces kvmclock difference on migration from 5s to 0.1s
> > > > > (when max_downtime == 5s).
> > > > >
> > > > > Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> > > > >
> > > > > ---
> > > > > hw/i386/kvm/clock.c | 108 ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > include/hw/i386/pc.h | 5 ++
> > > > > target-i386/kvm.c | 7 +++
> > > > > target-i386/kvm_i386.h | 1
> > > > > 4 files changed, 107 insertions(+), 14 deletions(-)
> > > > >
> > > > > v2:
> > > > > - improve variable names (Juan)
> > > > > - consolidate code on kvm_get_clock function (Paolo)
> > > > > - return mach_use_reliable_get_clock from needed function (Paolo)
> > > > >
> > > > > Index: qemu-mig-advance-clock/hw/i386/kvm/clock.c
> > > > > ===================================================================
> > > > > --- qemu-mig-advance-clock.orig/hw/i386/kvm/clock.c 2016-11-14 10:40:39.748116312 -0200
> > > > > +++ qemu-mig-advance-clock/hw/i386/kvm/clock.c 2016-11-14 13:38:29.299955042 -0200
> > > > > @@ -36,6 +36,12 @@
> > > > >
> > > > > uint64_t clock;
> > > > > bool clock_valid;
> > > > > +
> > > > > + /* whether machine supports reliable KVM_GET_CLOCK */
> > > > > + bool mach_use_reliable_get_clock;
> > > > > +
> > > > > + /* whether source host supported reliable KVM_GET_CLOCK */
> > > > > + bool src_use_reliable_get_clock;
> > > > > } KVMClockState;
> > > > >
> > > > > struct pvclock_vcpu_time_info {
> > > > > @@ -81,6 +87,19 @@
> > > > > return nsec + time.system_time;
> > > > > }
> > > > >
> > > > > +static uint64_t kvm_get_clock(void)
> > > > > +{
> > > > > + struct kvm_clock_data data;
> > > > > + int ret;
> > > > > +
> > > > > + ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> > > > > + if (ret < 0) {
> > > > > + fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> > > > > + abort();
> > > > > + }
> > > > > + return data.clock;
> > > > > +}
> > > > > +
> > > > > static void kvmclock_vm_state_change(void *opaque, int running,
> > > > > RunState state)
> > > > > {
> > > > > @@ -91,15 +110,37 @@
> > > > >
> > > > > if (running) {
> > > > > struct kvm_clock_data data = {};
> > > > > - uint64_t time_at_migration = kvmclock_current_nsec(s);
> > > > > + uint64_t pvclock_via_mem = 0;
> > > > >
> > > > > - s->clock_valid = false;
> > > > > + /* local (running VM) restore */
> > > > > + if (s->clock_valid) {
> > > > > + /*
> > > > > + * if host does not support reliable KVM_GET_CLOCK,
> > > > > + * read kvmclock value from memory
> > > > > + */
> > > > > + if (!kvm_has_adjust_clock_stable()) {
> > > > > + pvclock_via_mem = kvmclock_current_nsec(s);
> > > > > + }
> > > > > + /* migration/savevm/init restore */
> > > > > + } else {
> > > > > + /*
> > > > > + * use s->clock in case machine uses reliable
> > > > > + * get clock and source host supported
> > > > > + * reliable get clock
> > > > > + */
> > > > > + if (!(s->mach_use_reliable_get_clock &&
> > > > > + s->src_use_reliable_get_clock)) {
> > > > > + pvclock_via_mem = kvmclock_current_nsec(s);
> > > > > + }
> > > >
> > > > The s->mach_use_reliable_get_clock check seems redundant.
> > > > src_use_reliable_get_clock is set only if
> > > > mach_use_reliable_get_clock is true.
> > >
> > > Done.
> > >
> > > > > + }
> > > > >
> > > > > - /* We can't rely on the migrated clock value, just discard it */
> > > > > - if (time_at_migration) {
> > > > > - s->clock = time_at_migration;
> > > > > + /* We can't rely on the saved clock value, just discard it */
> > > > > + if (pvclock_via_mem) {
> > > > > + s->clock = pvclock_via_mem;
> > > > > }
> > > > >
> > > > > + s->clock_valid = false;
> > > > > +
> > > > > data.clock = s->clock;
> > > > > ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> > > > > if (ret < 0) {
> > > > > @@ -120,8 +161,6 @@
> > > > > }
> > > > > }
> > > > > } else {
> > > > > - struct kvm_clock_data data;
> > > > > - int ret;
> > > > >
> > > > > if (s->clock_valid) {
> > > > > return;
> > > > > @@ -129,13 +168,7 @@
> > > > >
> > > > > kvm_synchronize_all_tsc();
> > > > >
> > > > > - ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> > > > > - if (ret < 0) {
> > > > > - fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> > > > > - abort();
> > > > > - }
> > > > > - s->clock = data.clock;
> > > > > -
> > > > > + s->clock = kvm_get_clock();
> > > > > /*
> > > > > * If the VM is stopped, declare the clock state valid to
> > > > > * avoid re-reading it on next vmsave (which would return
> > > > > @@ -152,22 +185,69 @@
> > > > > qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> > > > > }
> > > > >
> > > > > +static bool kvmclock_src_use_reliable_get_clock(void *opaque)
> > > > > +{
> > > > > + KVMClockState *s = opaque;
> > > > > +
> > > > > + /*
> > > > > + * On machine types that support reliable KVM_GET_CLOCK,
> > > > > + * if host kernel does provide reliable KVM_GET_CLOCK,
> > > > > + * set src_use_reliable_get_clock=true so that destination
> > > > > + * avoids reading kvmclock from memory.
> > > > > + */
> > > > > + if (s->mach_use_reliable_get_clock && kvm_has_adjust_clock_stable()) {
> > > > > + s->src_use_reliable_get_clock = true;
> > > > > + }
> > > >
> > > > It feels fragile to change device state inside the .needed
> > > > function. Better to initialize src_use_reliable_get_clock on
> > > > kvmclock_realize()?
> > > >
> > > > What exactly ensures src_use_reliable_get_clock is correctly
> > > > initialized on the migration destination as well?
> > >
> > > Its initialized to false (because its part of device state).
> >
> > Do you mean it is always going to be false on the destination
> > host? What if .needed is called by other code? .needed shouldn't
> > have any side-effects.
>
> I don't think needed is called by any other code.
I don't know if it is called. But it may be called by other code,
so it can't have side-effects.
>
> > Also, the difference between the variables on the destination and
> > the source is very confusing, see other comments below:
>
> Yes it is: its because you don't know when is what. But if you consider
> separate events:
>
> initialization, incoming migration, vmstop, vmcontinue.
>
> And the values of the variables for each case, then it becomes
> clear.
>
> If anyone has a suggestion to improve clarify, its welcome.
I think it was confusing when src_use_reliable_get_clock was
initialized on .needed, but now it makes sense to me because
mach_use_reliable_get_clock and src_use_reliable_get_clock have
the same value on source and destination.
>
> > > >
> > > > > +
> > > > > + return s->mach_use_reliable_get_clock;
> > > >
> > > > If if kvm_has_adjust_clock_stable() is false, isn't it simpler to
> > > > simply skip the section?
> > > >
> > > > It looks like mach_use_reliable_get_clock and
> > > > src_use_reliable_get_clock could become a single field:
> > > >
> > > > * use_reliable_get_clock ("x-use-reliable-get-clock" property)
> > > > set to true by default (set on DEFINE_PROP_BOOL parameter)
> > > > * "x-use-reliable-get-clock" set to false by default on older
> > > > machine-types
> > > > * use_reliable_get_clock forced to false on kvmclock_realize() if
> > > > !kvm_has_adjust_clock_stable()
> > > > * kvmclock_reliable_get_clock.needed return s->use_reliable_get_clock
> > >
> > > No because use_reliable_get_clock can be false because the destination host
> > > does not support kvm_has_adjust_clock_stable(), but the source host
> > > supports it (so we want to use the value in the first incoming
> > > migration).
> > >
> >
> > Are you talking about the value of the fields on the source host,
> > or on the destination host? In this case, you mean that
> > use_reliable_get_clock should be false on the destination, right?
>
> Yes, destination.
>
> > > So the variable indicates whether the source host supported
> > > migration, not whether the destination supports it.
> >
> > Good, that's the whole point of the field, isn't it? I will try
> > to summarize all cases below. Please correct me if something is
> > incorrect.
>
> Yes. So in this case, the cleanup/simplification to use a single
> variable is not possible (or maybe it is, but it just complicates
> things).
>
> > --------+------------+---------------+-----------+------------+
> > | kvm_has_adj_clock_stable() | use_reliable_get_clock |
> > machine | src | dst | src | dst |
> > --------+------------+---------------+-----------+------------+
> > pc-2.8 | false | false | false | false |
> > pc-2.8 | false | true | false | false (*) |
> > pc-2.8 | true | false | true | true (**) |
> > pc-2.8 | true | true | true | true |
> > --------+------------+---------------+-----------+------------+
> > pc-2.7 | (any) | (any) | false | false |
> > --------+------------+---------------+-----------+------------+
> >
> > (*) I guess this is where things would break if we skip the
> > subsection (and use a single field). If the subsection is
> > skipped, use_reliable_get_clock would be set to true on the
> > destination. (See my comment below on the new patch you sent)
>
> Well, perhaps you can massage things so that it works
> without sending subsection (that is):
>
> * Send subsection only in case value is true.
> * Do not send subsection in case value is false (then treat
> non presence of subsection and mach_reliable_clock=true
> to mean source did not support kvm_has_reliable_clock).
Yes, it would be more complex. Always sending it if
mach_use_reliable_get_clock=true (like you proposed) is simpler.
>
> Once you agree on "send a subsection for both cases", then you
> handle it.
>
> The code is there, its simple, readable, so...
>
> (what you can't do is to unify src_use_get_reliable_clock (remote
> information), and local information). Because you need
> to know both things to be able to handle the 4 cases properly on
> incoming migration.
OK.
> [...]
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
