qemu: Limit refresh of CPU halted state to s390

[libvirt] [PATCH] qemu: Limit refresh of CPU halted state to s390

Posted by Viktor Mihajlovski 6 years, 2 months ago

Refreshing the halted state can cause VM performance issues. Since
s390 is currently the only architecture with a known interest in
the halted state, we're avoiding to call QEMU on other platforms.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 src/qemu/qemu_domain.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index df433c2..d2c833f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8634,6 +8634,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
     if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
         return 0;
 
+    /* Only supported on s390(x) */
+    if (!ARCH_IS_S390(vm->def->os.arch))
+        return 0;
+
     if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
         return -1;
 
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Limit refresh of CPU halted state to s390

Posted by Peter Krempa 6 years, 2 months ago

On Tue, Feb 06, 2018 at 11:18:56 +0100, Viktor Mihajlovski wrote:
> Refreshing the halted state can cause VM performance issues. Since
> s390 is currently the only architecture with a known interest in
> the halted state, we're avoiding to call QEMU on other platforms.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_domain.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index df433c2..d2c833f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8634,6 +8634,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
>      if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
>          return 0;
>  
> +    /* Only supported on s390(x) */
> +    if (!ARCH_IS_S390(vm->def->os.arch))
> +        return 0;

I think we also should remove the 'halted' field from the stats output
if the information was not gathered at all, since it would falsely reply
that the cpu is not halted all the time.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Limit refresh of CPU halted state to s390

Posted by Peter Krempa 6 years, 2 months ago

On Tue, Feb 06, 2018 at 11:55:07 +0100, Peter Krempa wrote:
> On Tue, Feb 06, 2018 at 11:18:56 +0100, Viktor Mihajlovski wrote:
> > Refreshing the halted state can cause VM performance issues. Since
> > s390 is currently the only architecture with a known interest in
> > the halted state, we're avoiding to call QEMU on other platforms.
> > 
> > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> > ---
> >  src/qemu/qemu_domain.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index df433c2..d2c833f 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -8634,6 +8634,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
> >      if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
> >          return 0;
> >  
> > +    /* Only supported on s390(x) */

This would also benefit from a more thorough comment.

> > +    if (!ARCH_IS_S390(vm->def->os.arch))
> > +        return 0;
> 
> I think we also should remove the 'halted' field from the stats output
> if the information was not gathered at all, since it would falsely reply
> that the cpu is not halted all the time.

Also to do so you'll probably need to convert the 'halted' bool to a
tristate, so that the logic determining whether the field should be used
or not is not present in the stats code but it's in the refresh code as
you've added it.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Limit refresh of CPU halted state to s390

Posted by Daniel P. Berrangé 6 years, 2 months ago

On Tue, Feb 06, 2018 at 12:00:31PM +0100, Peter Krempa wrote:
> On Tue, Feb 06, 2018 at 11:55:07 +0100, Peter Krempa wrote:
> > On Tue, Feb 06, 2018 at 11:18:56 +0100, Viktor Mihajlovski wrote:
> > > Refreshing the halted state can cause VM performance issues. Since
> > > s390 is currently the only architecture with a known interest in
> > > the halted state, we're avoiding to call QEMU on other platforms.
> > > 
> > > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> > > ---
> > >  src/qemu/qemu_domain.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index df433c2..d2c833f 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -8634,6 +8634,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
> > >      if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
> > >          return 0;
> > >  
> > > +    /* Only supported on s390(x) */
> 
> This would also benefit from a more thorough comment.
> 
> > > +    if (!ARCH_IS_S390(vm->def->os.arch))
> > > +        return 0;
> > 
> > I think we also should remove the 'halted' field from the stats output
> > if the information was not gathered at all, since it would falsely reply
> > that the cpu is not halted all the time.
> 
> Also to do so you'll probably need to convert the 'halted' bool to a
> tristate, so that the logic determining whether the field should be used
> or not is not present in the stats code but it's in the refresh code as
> you've added it.

Or the lazy way out is to just add ARCH_IS_S390() check in the stats
method too


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Limit refresh of CPU halted state to s390

Posted by Peter Krempa 6 years, 2 months ago

On Tue, Feb 06, 2018 at 11:05:06 +0000, Daniel Berrange wrote:
> On Tue, Feb 06, 2018 at 12:00:31PM +0100, Peter Krempa wrote:
> > On Tue, Feb 06, 2018 at 11:55:07 +0100, Peter Krempa wrote:
> > > On Tue, Feb 06, 2018 at 11:18:56 +0100, Viktor Mihajlovski wrote:
> > > > Refreshing the halted state can cause VM performance issues. Since
> > > > s390 is currently the only architecture with a known interest in
> > > > the halted state, we're avoiding to call QEMU on other platforms.
> > > > 
> > > > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> > > > ---
> > > >  src/qemu/qemu_domain.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index df433c2..d2c833f 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -8634,6 +8634,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
> > > >      if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
> > > >          return 0;
> > > >  
> > > > +    /* Only supported on s390(x) */
> > 
> > This would also benefit from a more thorough comment.
> > 
> > > > +    if (!ARCH_IS_S390(vm->def->os.arch))
> > > > +        return 0;
> > > 
> > > I think we also should remove the 'halted' field from the stats output
> > > if the information was not gathered at all, since it would falsely reply
> > > that the cpu is not halted all the time.
> > 
> > Also to do so you'll probably need to convert the 'halted' bool to a
> > tristate, so that the logic determining whether the field should be used
> > or not is not present in the stats code but it's in the refresh code as
> > you've added it.
> 
> Or the lazy way out is to just add ARCH_IS_S390() check in the stats
> method too

No. That's exactly why I've sent this second mail. I don't want to
litter this stuff into the stats method. The stats method should not
care what weirdness is necessary for a given arch.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list