Re: [QEMU PATCH] kvmclock: advance clock by time window between vm_stop and pre_save

Paolo Bonzini
2016-11-04T15:58:05Z

On 04/11/2016 16:48, Radim Krčmář wrote:
> 2016-11-04 16:33+0100, Paolo Bonzini:
>> On 04/11/2016 16:25, Radim Krčmář wrote:
>>>>>
>>>>> + if (s->advance_clock && s->clock + s->advance_clock > s->clock) {
>>>>> + s->clock += s->advance_clock;
>>>>> + s->advance_clock = 0;
>>>>> + }
>>> Can't the advance_clock added to the migrated KVMClockState instead of
>>> passing it as another parameter?
>>>
>>> (It is sad that we can't just query KVMClockState in kvmclock_pre_save
>>> because of the Linux bug.)
>>
>> What Linux bug? The one that makes us use kvmclock_current_nsec?
>
> No, the one that forced Marcelo to add the 10 minute limit to the
> advance_clock. We wouldn't need this advance_clock hack if we could
> just call KVM_GET_CLOCK like we did before 00f4d64ee76e ("kvmclock:
> clock should count only if vm is running").

There are two cases:

- migrating a paused guest

- pausing at the end of migration

In the first case, kvmclock_vm_state_change's !running branch will see
state == RUN_STATE_FINISH_MIGRATE && s->clock_valid. In the second
case, it will see state == RUN_STATE_FINISH_MIGRATE && !s->clock_valid.

In the second case it should do nothing. Then kvmclock_pre_save will
see !s->clock_valid and call KVM_GET_CLOCK. A bonus is that, if
migration fails and migration_thread() calls vm_start(), then the guest
will not see the clock drift backwards.

>> It should work with 4.9-rc (well, once Linus applies my pull request).
>> 4.9-rc will not return ktime_get_ns for KVM_GET_CLOCK; it will return
>> the raw value from the kernel timekeeper.
>>
>> I'm thinking that we should add a KVM capability for this, and skip
>> kvmclock_current_nsec if the capability is present. The first part is
>> trivial, so we can do it even during Linux rc period.
>
> I agree, that sounds like a nice improvement.

Ok, I'll send the KVM patch then. It can even be the value *returned*
for KVM_CAP_ADJUST_CLOCK---right now it returns 1, we can return 3 (bit
0 for KVM_CAP_ADJUST_CLOCK_RESENT and bit 1 for returning the raw
value). Any suggestion for the name?

Paolo