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

[Paolo Bonzini]

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.

I'm thinking "mach_use_reliable_get_clock is just for migration,
src_use_reliable_get_clock is the state".  Perhaps you're thinking of
enabling/disabling the whole new code for old machines?  What is the
advantage?

>>> +            }
>>> +        }
>>>  
>>> -        /* 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.  If you return true here, you can still
migrate a "false" value for src_use_reliable_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, and then the idea is to treat
clock,src_use_reliable_get_clock as one tuple that is updated atomically.

Paolo