Re: [Qemu-devel] [qemu patch 2/2] kvmclock: reduce kvmclock difference on migration

[Marcelo Tosatti]

On Mon, Nov 14, 2016 at 03:22:02PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 15:00, Marcelo Tosatti wrote:
> > On Mon, Nov 14, 2016 at 02:54:38PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 14/11/2016 13:36, Marcelo Tosatti wrote:
> >>> +        /* 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()) {
> >>> +                time_at_migration = kvmclock_current_nsec(s);
> >>
> >> Just assign to s->clock here...
> > 
> > If kvmclock is not enabled, you want to use s->clock,
> > rather than 0.
> > 
> >>> +            }
> >>> +        /* migration/savevm/init restore */
> >>> +        } else {
> >>> +            /*
> >>> +             * use s->clock in case machine uses reliable
> >>> +             * get clock and host where vm was executing
> >>> +             * supported reliable get clock
> >>> +             */
> >>> +            if (!s->mach_use_reliable_get_clock ||
> >>> +                !s->src_use_reliable_get_clock) {
> >>> +                time_at_migration = kvmclock_current_nsec(s);
> >>
> >> ... and here, so that time_at_migration is not needed anymore.
> > 
> > Same as above.
> 
> You're right.
> 
> >> Also here it's enough to look at s->src_user_reliable_get_clock, because
> >> if s->mach_use_reliable_get_clock is false,
> >> s->src_use_reliable_get_clock will be false as well.
> > 
> > Yes, but i like the code annotation.
> 
> Ah, I think we're looking at it differently.

Well, i didnt want to mix the meaning of the variables:

+    /* 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;

See the comments on top (later if you look at the variable, 
then have to think: well it has one name, but its disabled 
by that other path as well, so its more than its 
name,etc...).

> I'm thinking "mach_use_reliable_get_clock is just for migration,

Thats whether the machine supports it. New machines have it enabled,
olders don't.
> src_use_reliable_get_clock is the state". 

Thats whether the migration source supported it.

> Perhaps you're thinking of
> enabling/disabling the whole new code for old machines? 

source          destination         behaviour
supports        supports            on migration use s->clock,
                                    on stop/cont as well

supports        ~supports
                                    on migration use s->clock,
                                    on stop/cont read from guest mem

~support        supports            on migration read from guest,
                                    on stop/cont use
                                    kvm_get_clock/kvm_set_clock

~support        ~support            always read from guest memory


Thats what should happen (and thats what the patch should implement).


"support" means host supports your new KVM_GET_CLOCK/KVM_SET_CLOCK.

>  What is the
> advantage?

Well its necessary to use the correct thing, otherwise
you see a time backwards event.

> 
> >>> +            }
> >>> +        }
> >>>  
> >>> -        /* We can't rely on the migrated clock value, just discard it */
> >>> +        /* We can't rely on the saved clock value, just discard it */
> >>>          if (time_at_migration) {
> >>>              s->clock = time_at_migration;
> >>
> >> [...]
> >>
> >>>
> >>> +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;
> >>> +    }
> >>> +
> >>> +    return s->src_use_reliable_get_clock;
> >>> +}
> >>
> >> Here you can just return s->mach_use_reliable_get_clock. 
> > 
> > mach_use_reliable_get_clock can be true but host might not support it.
> 
> Yes, but the "needed" function is only required to avoid breaking
> pc-i440fx-2.7 and earlier. 

"needed" is required so that the migration between:

SRC             DEST                BEHAVIOUR
~support        supports            on migration read from guest,
                                    on stop/cont use
                                    kvm_get_clock/kvm_set_clock

Destination does not use KVM_GET_CLOCK value (which is
broken and should not be used).

> If you return true here, you can still
> migrate a "false" value for src_use_reliable_get_clock.

But the source only uses a reliable KVM_GET_CLOCK if 
both conditions are true.

And the subsection is only needed if the source
uses a reliable KVM_GET_CLOCK.

> >>  To set
> >> s->src_use_reliable_get_clock, after issuing KVM_GET_CLOCK you can look
> >> at the KVM_CLOCK_TSC_STABLE bit in the kvm_clock struct's flags.
> > 
> > KVM_CLOCK_TSC_STABLE bit in the kvmclock structure != 
> > KVM_GET_CLOCK returns reliable value, right?
> 
> It is the same as "is using masterclock", which is actually a stricter
> condition than the KVM_CHECK_EXTENSION return value.  The right check to
> use is whether masterclock is in use, 

Actually its "has a reliable KVM_GET_CLOCK" (which returns 
get_kernel_clock() + (rdtsc() - tsc_timestamp), 

"broken KVM_GET_CLOCK" =  get_kernel_clock()

> and then the idea is to treat
> clock,src_use_reliable_get_clock as one tuple that is updated atomically.
> 
> Paolo

Hum, not sure i get this...