[Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Li, Liang Z]

Hi Paolo & Marcelo,

    Could please point out what issue the patch 317b0a6d8ba44e try to fix?  I found in live migration the cpu_synchronize_all_states will be called twice, and it will take more than 1 ms sometimes. I try to do some optimization but lack the knowledge about the background. 

Thanks

Liang

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Marcelo Tosatti]

On Thu, Aug 13, 2015 at 01:25:29AM +0000, Li, Liang Z wrote:
> Hi Paolo & Marcelo,
> 
>     Could please point out what issue the patch 317b0a6d8ba44e try to fix?  I found in live migration the cpu_synchronize_all_states will be called twice, and it will take more than 1 ms sometimes. I try to do some optimization but lack the knowledge about the background. 

What the code in 317b0a6d8ba44e requires is to retrieve the TSC value
from the kernel.

Perhaps you can trace kvm_arch_get_regs and look why its taking that
amount of time and what can be done to improve it.

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Li, Liang Z]

> On Thu, Aug 13, 2015 at 01:25:29AM +0000, Li, Liang Z wrote:
> > Hi Paolo & Marcelo,
> >
> >     Could please point out what issue the patch 317b0a6d8ba44e try to fix?  I
> found in live migration the cpu_synchronize_all_states will be called twice,
> and it will take more than 1 ms sometimes. I try to do some optimization but
> lack the knowledge about the background.
> 
> What the code in 317b0a6d8ba44e requires is to retrieve the TSC value from
> the kernel.

I know 317b0a6d8ba44e is to retrieve the TSC value, but I don't understand why it is needed. During the live migration, the cpu_synchronize_all_states will be called later after stopping kvm-clock. The env->tsc will be updated, is that not enough? Or is there some case like call the 'stop_vm(RUN_STATE_PAUSED )' or ' 'stop_vm (RUN_STATE_DEBUG) ', that require updating the env->tsc? By google, I find that your patch try to fix some issue, but I don't know what the exact issue.

Anyway, using cpu_synchronize_all_states to update the env->tsc is a littler expensive, especially in the stop can copy stage.

Liang

> Perhaps you can trace kvm_arch_get_regs and look why its taking that
> amount of time and what can be done to improve it.

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Marcin Gibuła]

W dniu 2015-08-14 o 03:23, Li, Liang Z pisze:
>> On Thu, Aug 13, 2015 at 01:25:29AM +0000, Li, Liang Z wrote:
>>> Hi Paolo & Marcelo,
>>>
>>>      Could please point out what issue the patch 317b0a6d8ba44e try to fix?  I
>> found in live migration the cpu_synchronize_all_states will be called twice,
>> and it will take more than 1 ms sometimes. I try to do some optimization but
>> lack the knowledge about the background.
>>
>> What the code in 317b0a6d8ba44e requires is to retrieve the TSC value from
>> the kernel.
>
> I know 317b0a6d8ba44e is to retrieve the TSC value, but I don't understand why it is needed. During the live migration, the cpu_synchronize_all_states will be called later after stopping kvm-clock. The env->tsc will be updated, is that not enough? Or is there some case like call the 'stop_vm(RUN_STATE_PAUSED )' or ' 'stop_vm (RUN_STATE_DEBUG) ', that require updating the env->tsc? By google, I find that your patch try to fix some issue, but I don't know what the exact issue.

I remember testing these, and I afair that was the reason:

http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00472.html

-- 
mg

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Li, Liang Z]

> >>>      Could please point out what issue the patch 317b0a6d8ba44e try
> >>> to fix?  I
> >> found in live migration the cpu_synchronize_all_states will be called
> >> twice, and it will take more than 1 ms sometimes. I try to do some
> >> optimization but lack the knowledge about the background.
> >>
> >> What the code in 317b0a6d8ba44e requires is to retrieve the TSC value
> >> from the kernel.
> >
> > I know 317b0a6d8ba44e is to retrieve the TSC value, but I don't understand
> why it is needed. During the live migration, the cpu_synchronize_all_states
> will be called later after stopping kvm-clock. The env->tsc will be updated, is
> that not enough? Or is there some case like call the
> 'stop_vm(RUN_STATE_PAUSED )' or ' 'stop_vm (RUN_STATE_DEBUG) ', that
> require updating the env->tsc? By google, I find that your patch try to fix
> some issue, but I don't know what the exact issue.
> 
> I remember testing these, and I afair that was the reason:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00472.html
> 
> --
> mg

Hi Mg,

    Thanks for your reply, I have read the thread in your email, what's the mean of 'switching from old to new disk', could give a detail description? 

Liang 

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Marcin Gibuła]

>      Thanks for your reply, I have read the thread in your email, what's the mean of 'switching from old to new disk', could give a detail description?

The test case was like that (using libvirt):

1. Get VM running (linux, using kvmclock),
2. Use blockcopy to copy disk data from one location to another,
3. Issue blockjob --pivot (to finish mirroring)

 From what I remember, at point 3, VM is momentarily paused and resumed, 
so kvm state change handler is called twice. Without this patch, the VM 
hanged because its time goes backwards (or qemu crashed if assertion was 
not compiled out).

-- 
mg

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Li, Liang Z]

> Subject: Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc
> value for kvmclock_current_nsec calculation
> 
> >      Thanks for your reply, I have read the thread in your email, what's the
> mean of 'switching from old to new disk', could give a detail description?
> 
> The test case was like that (using libvirt):
> 
> 1. Get VM running (linux, using kvmclock), 2. Use blockcopy to copy disk data
> from one location to another, 3. Issue blockjob --pivot (to finish mirroring)
> 
>  From what I remember, at point 3, VM is momentarily paused and resumed,
> so kvm state change handler is called twice. Without this patch, the VM
> hanged because its time goes backwards (or qemu crashed if assertion was
> not compiled out).
> 
> --
> mg

So, the problem is cause by stop_vm(RUN_STATE_PAUSED), in this case the env->tsc is not updated, which lead to the issue. 
Is that right?  If the cpu_clean_all_dirty() is needed just for the APIC status reason, I think we can do the cpu_synchronize_all_states() in do_vm_stop
and after vm_state_notify() when the RUN_STATE_PAUSED is hit, at this point all the device models is stopped, there is no outdated APIC status.  

I want to write a patch to fix this issue in another way, could help to verify it in you environment, very appreciate if you could.

Thanks.

Liang

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Marcin Gibuła]

> So, the problem is cause by stop_vm(RUN_STATE_PAUSED), in this case the env->tsc is not updated, which lead to the issue.
> Is that right?

I think so.

> If the cpu_clean_all_dirty() is needed just for the APIC status reason, I think we can do the cpu_synchronize_all_states() in do_vm_stop
> and after vm_state_notify() when the RUN_STATE_PAUSED is hit, at this point all the device models is stopped, there is no outdated APIC status.

Yes, cpu_clean_all_dirty() was needed because without it, the second 
call to cpu_synchronize_all_states() (which is done inside 
qemu_savevm_state_complete() and after kvmclock) does nothing.

> I want to write a patch to fix this issue in another way, could help to verify it in you environment, very appreciate if you could.

Sure, I'll test it. Both issues were quite easy to reproduce.

-- 
mg

Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc value for kvmclock_current_nsec calculation

[Marcelo Tosatti]

On Fri, Aug 14, 2015 at 09:18:01AM +0000, Li, Liang Z wrote:
> > Subject: Re: [Qemu-devel] about the patch kvmclock Ensure proper env->tsc
> > value for kvmclock_current_nsec calculation
> > 
> > >      Thanks for your reply, I have read the thread in your email, what's the
> > mean of 'switching from old to new disk', could give a detail description?
> > 
> > The test case was like that (using libvirt):
> > 
> > 1. Get VM running (linux, using kvmclock), 2. Use blockcopy to copy disk data
> > from one location to another, 3. Issue blockjob --pivot (to finish mirroring)
> > 
> >  From what I remember, at point 3, VM is momentarily paused and resumed,
> > so kvm state change handler is called twice. Without this patch, the VM
> > hanged because its time goes backwards (or qemu crashed if assertion was
> > not compiled out).
> > 
> > --
> > mg
> 
> So, the problem is cause by stop_vm(RUN_STATE_PAUSED), in this case the env->tsc is not updated, which lead to the issue. 
> Is that right?  If the cpu_clean_all_dirty() is needed just for the APIC status reason, I think we can do the cpu_synchronize_all_states() in do_vm_stop
> and after vm_state_notify() when the RUN_STATE_PAUSED is hit, at this point all the device models is stopped, there is no outdated APIC status.  
> 
> I want to write a patch to fix this issue in another way, could help to verify it in you environment, very appreciate if you could.
> 
> Thanks.
> 
> Liang

Liang,

The problem is this:

Think two hosts, source and destination. The code is running in the
destination host.

The clock which the source host maintains (CLOCK_MONOTONIC), which is what
KVM_GET_CLOCK ioctl uses, is corrected by NTP.
The clock which the guest uses can be based on TSC, which can drift
relative to UTC. 

So at the moment the guest is stopped at the source, KVM_GET_CLOCK clock
(s->clock in kvmclock.c) might be behind of TSC based clock (which means
that setting that clock value at the destination would case the guest to
see time going backwards which is fatal).

What the code does is to, on the destination host, read the clock value
from memory rather than to use what has been retrieved in the source
host with KVM_GET_CLOCK.

For that, it needs uptodated env->tsc value relative to the destination
host (TSC+TSC_OFFSET of destination host).

Yes i can test your patch.