Re: [qemu patch V4 2/2] kvmclock: reduce kvmclock difference on migration

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Dec 12, 2016 at 05:44:52PM -0200, Marcelo Tosatti wrote:
> On Mon, Dec 12, 2016 at 04:01:05PM -0200, Eduardo Habkost wrote:
> > On Sat, Dec 10, 2016 at 03:21:50PM -0200, Marcelo Tosatti wrote:
> > [...]
> > >  static void kvmclock_realize(DeviceState *dev, Error **errp)
> > >  {
> > >      KVMClockState *s = KVM_CLOCK(dev);
> > >  
> > > +    if (kvm_has_adjust_clock_stable()) {
> > > +        s->clock_is_reliable = true;
> > > +    }
> > > +
> > 
> > This seems unnecessary, as kvmclock_vm_state_change() makes sure
> > it is set at the same time as s->clock. Should we just remove it?
> 
> There is this initialization that goes from ~running -> running
> which assumes its initialized:

Right: I forgot about the very first time
kvmclock_vm_state_change() is called.

It doesn't seem to make any difference (as both s->clock
kvmclock_current_nsec() will return 0 anyway), but at least it
makes clock_is_reliable consistent with its documented purpose.

I would simplify it to a single line:
    s->clock_is_reliable = kvm_has_adjust_clock_stable();

But it is not a big deal, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks!

> 
> static void kvmclock_vm_state_change(void *opaque, int running,
>                                      RunState state)
> {
>     KVMClockState *s = opaque;
>     CPUState *cpu;
>     int cap_clock_ctrl = kvm_check_extension(kvm_state,
> KVM_CAP_KVMCLOCK_CTRL);
>     int ret;
> 
>     if (running) {
>         struct kvm_clock_data data = {};
>         uint64_t pvclock_via_mem = 0;
> 
>         /*
>          * If the host where s->clock was read did not support reliable
>          * KVM_GET_CLOCK, read kvmclock value from memory.
>          */
>         if (!s->clock_is_reliable) {
>             pvclock_via_mem = kvmclock_current_nsec(s);
>         }
> 

-- 
Eduardo