Re: [PATCH 0/4] Avoid soft lockup message when KVM is stopped by host

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Sep 20, 2011 at 03:00:54PM -0400, Eric B Munson wrote:
> On Thu, 15 Sep 2011, Marcelo Tosatti wrote:
> 
> > On Tue, Sep 13, 2011 at 04:49:55PM -0400, Eric B Munson wrote:
> > > On Fri, 09 Sep 2011, Marcelo Tosatti wrote:
> > > 
> > > > On Thu, Sep 01, 2011 at 02:27:49PM -0600, emunson@mgebm.net wrote:
> > > > > On Thu, 01 Sep 2011 14:24:12 -0500, Anthony Liguori wrote:
> > > > > >On 08/30/2011 07:26 AM, Marcelo Tosatti wrote:
> > > > > >>On Mon, Aug 29, 2011 at 05:27:11PM -0600, Eric B Munson wrote:
> > > > > >>>Currently, when qemu stops a guest kernel that guest will
> > > > > >>>issue a soft lockup
> > > > > >>>message when it resumes.  This set provides the ability for
> > > > > >>>qemu to comminucate
> > > > > >>>to the guest that it has been stopped.  When the guest hits
> > > > > >>>the watchdog on
> > > > > >>>resume it will check if it was suspended before issuing the
> > > > > >>>warning.
> > > > > >>>
> > > > > >>>Eric B Munson (4):
> > > > > >>>   Add flag to indicate that a vm was stopped by the host
> > > > > >>>   Add functions to check if the host has stopped the vm
> > > > > >>>   Add generic stubs for kvm stop check functions
> > > > > >>>   Add check for suspended vm in softlockup detector
> > > > > >>>
> > > > > >>>  arch/x86/include/asm/pvclock-abi.h |    1 +
> > > > > >>>  arch/x86/include/asm/pvclock.h     |    2 ++
> > > > > >>>  arch/x86/kernel/kvmclock.c         |   14 ++++++++++++++
> > > > > >>>  include/asm-generic/pvclock.h      |   14 ++++++++++++++
> > > > > >>>  kernel/watchdog.c                  |   12 ++++++++++++
> > > > > >>>  5 files changed, 43 insertions(+), 0 deletions(-)
> > > > > >>>  create mode 100644 include/asm-generic/pvclock.h
> > > > > >>>
> > > > > >>>--
> > > > > >>>1.7.4.1
> > > > > >>
> > > > > >>How is the host supposed to set this flag?
> > > > > >>
> > > > > >>As mentioned previously, if you save save/restore the offset
> > > > > >>added to
> > > > > >>kvmclock on stop/cont (and the TSC MSR, forgot to mention that), no
> > > > > >>paravirt infrastructure is required. Which means the issue is
> > > > > >>also fixed
> > > > > >>for older guests.
> > > > > >
> > > 
> > > Marcelo,
> > > 
> > > I think that stopping the TSC is the wrong approach because it will break time
> > > between the two systems so timething that expects the monotonic clock to move
> > > consistently will be wrong.
> > 
> > In case the VM stops for whatever reason, the host system is not
> > supposed to adjust time related hardware state to compensate, in an
> > attempt to present apparent continuous time.
> > 
> > If you save a VM and then restore it later, it is the guest
> > responsability to adjust its time representation.
> > 
> > QEMU exposing continuous TSC and kvmclock state between "stop" and
> > "cont" should not be a reason to introduce new paravirt infrastructure.
> > 
> > >  IMO, messing with the TSC at run time to avoid a watchdog message
> > > is the wrong solution, better to teach the watchdog to ignore this
> > > special case.
> > 
> > OK then, it is not a harmful addition, can you post the QEMU patches to
> > set the "ignore watchdog" bit.
> 
> I am about to start working on the interface to set this bit, but to double
> check I tried your suggestion to save the clock value on stop and restore it on
> continue (see qemu patch below) and I am seeing very odd behavior from it.  On
> random resumes the vm won't come back immediately (it took one 15 mintues to
> respond) 

You should also save/restore the TSC MSR for each VCPU. And it appears
there is a missing

kvm_for_each_vcpu(vcpu)
    kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu)

in KVM_SET_CLOCK ioctl handling.

> and the wall clock stays behind my host wall clock by the amount of
> time it took to resume.

This is expected, similar to savevm/loadvm.

> ---
>  cpus.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 54c188c..4573e23 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -63,6 +63,7 @@
>  #endif /* CONFIG_LINUX */
>  
>  static CPUState *next_cpu;
> +struct kvm_clock_data data;
>  
>  /***********************************************************/
>  void hw_error(const char *fmt, ...)
> @@ -788,6 +789,7 @@ static int all_vcpus_paused(void)
>  void pause_all_vcpus(void)
>  {
>      CPUState *penv = first_cpu;
> +    int ret;
>  
>      while (penv) {
>          penv->stop = 1;
> @@ -803,11 +805,18 @@ void pause_all_vcpus(void)
>              penv = (CPUState *)penv->next_cpu;
>          }
>      }
> +
> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> +    if (ret < 0) {
> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> +        data.clock = 0;
> +    }
>  }
>  
>  void resume_all_vcpus(void)
>  {
>      CPUState *penv = first_cpu;
> +    int ret;
>  
>      while (penv) {
>          penv->stop = 0;
> @@ -815,6 +824,11 @@ void resume_all_vcpus(void)
>          qemu_cpu_kick(penv);
>          penv = (CPUState *)penv->next_cpu;
>      }
> +    if (data.clock != 0) {
> +        ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> +        if (ret < 0)
> +            fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret));
> +    }
>  }
>  
>  static void qemu_tcg_init_vcpu(void *_env)
> >