Re: [Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
On 02/14/2018 09:16 AM, Eric Blake wrote: How shall we proceed with this series? Patch 3 depends upon patch 1, so I think it makes sense to merge this in one go. I can give my R-b on patch 1 and Someone(tm) can merge this, or I can take the whole series through the s390 tree (with some further reviews/acks on patches 2/3). I'd still like to give a thorough QMP review; it's on my list to get to today. Okay, I've added more review comments; I think there's still enough worth polishing that resending a v3 of the series is worthwhile, and do agree that we want all three patches through a single tree (s390 sound fine to me). But in general, I'm okay with the QMP changes (what looks like duplication is caused by modernizing the naming, which means we DON'T want to play games with trying to inherit one type from the other, and the argument about deprecating the old naming seems reasonable). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
On Wed, 14 Feb 2018 09:16:15 -0600 Eric Blake wrote: > On 02/14/2018 04:57 AM, Cornelia Huck wrote: > > On Tue, 13 Feb 2018 18:18:45 +0100 > > Viktor Mihajlovski wrote: > > > > >> Patch 1/3: > >>Adds architecture specific data to the QMP CpuInfo type, exposing > >>the existing s390 cpu-state in QMP. The cpu-state is a representation > >>more adequate than the ambiguous 'halted' condition. > >> > >> Patch 2/3: > >>Adds a new QMP function query-cpus-fast, which will only retrieve > >>vCPU information that can be obtained without interrupting the > >>vCPUs of a running VM. It introduces a new return type CpuInfoFast > >>with the subset of fields meeting this condition. Specifically, the > >>halted state is not part of CpuInfoFast. QMP clients like libvirt > >>are encouraged to switch to the new API for vCPU information. > >> > >> Patch 3/3: > >>Adds the s390-specific cpu state to CpuInfoFast, allowing management > >>apps to find out whether a vCPU is in the stopped state. This extension > >>leads to a partial duplication of field definitions from CpuInfo > >>to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and > >>eventually removed. > > > > How shall we proceed with this series? Patch 3 depends upon patch 1, so > > I think it makes sense to merge this in one go. > > > > I can give my R-b on patch 1 and Someone(tm) can merge this, or I can > > take the whole series through the s390 tree (with some further > > reviews/acks on patches 2/3). > > I'd still like to give a thorough QMP review; it's on my list to get to > today. > Sure, no need to hurry.
Re: [Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
* Viktor Mihajlovski (mihaj...@linux.vnet.ibm.com) wrote: > On 14.02.2018 11:57, Cornelia Huck wrote: > [...] > > > > How shall we proceed with this series? Patch 3 depends upon patch 1, so > > I think it makes sense to merge this in one go. > > > > I can give my R-b on patch 1 and Someone(tm) can merge this, or I can > > take the whole series through the s390 tree (with some further > > reviews/acks on patches 2/3). > > > Let's wait for the outstanding reviews (QAPI/HMP). If no major concerns > are voiced, the series could go in via the s390 tree. > As David H. pointed out, I've missed his r-b on patch 1, so you could > add that along with Christian's if no v3 is required. I'm happy with the HMP changes in teh series; they're only minor, so Acked-by: Dr. David Alan Gilbert > > -- > Regards, > Viktor Mihajlovski > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
On 02/14/2018 04:57 AM, Cornelia Huck wrote: On Tue, 13 Feb 2018 18:18:45 +0100 Viktor Mihajlovski wrote: Patch 1/3: Adds architecture specific data to the QMP CpuInfo type, exposing the existing s390 cpu-state in QMP. The cpu-state is a representation more adequate than the ambiguous 'halted' condition. Patch 2/3: Adds a new QMP function query-cpus-fast, which will only retrieve vCPU information that can be obtained without interrupting the vCPUs of a running VM. It introduces a new return type CpuInfoFast with the subset of fields meeting this condition. Specifically, the halted state is not part of CpuInfoFast. QMP clients like libvirt are encouraged to switch to the new API for vCPU information. Patch 3/3: Adds the s390-specific cpu state to CpuInfoFast, allowing management apps to find out whether a vCPU is in the stopped state. This extension leads to a partial duplication of field definitions from CpuInfo to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and eventually removed. How shall we proceed with this series? Patch 3 depends upon patch 1, so I think it makes sense to merge this in one go. I can give my R-b on patch 1 and Someone(tm) can merge this, or I can take the whole series through the s390 tree (with some further reviews/acks on patches 2/3). I'd still like to give a thorough QMP review; it's on my list to get to today. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
On 14.02.2018 11:57, Cornelia Huck wrote: [...] > > How shall we proceed with this series? Patch 3 depends upon patch 1, so > I think it makes sense to merge this in one go. > > I can give my R-b on patch 1 and Someone(tm) can merge this, or I can > take the whole series through the s390 tree (with some further > reviews/acks on patches 2/3). > Let's wait for the outstanding reviews (QAPI/HMP). If no major concerns are voiced, the series could go in via the s390 tree. As David H. pointed out, I've missed his r-b on patch 1, so you could add that along with Christian's if no v3 is required. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
On Tue, 13 Feb 2018 18:18:45 +0100 Viktor Mihajlovski wrote: > This series consolidates patches around a performance issue > caused by the usage of QMP query-cpus. > > A performance issue was found in an OpenStack environment, where > ceilometer was collecting domain statistics with libvirt. The domain > statistics reported by libvirt include the vCPU halted state, which > in turn is retrieved with QMP query-cpus. > > This causes two issues: > 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl >to find out whether a vCPU was halted. This is not the case for s390 >but query-cpus is always causing the vCPU to exit the VM. > > 2. Semantics: on x86 and other architectures, halted is a highly transient >state, which is likely to have already changed shortly after the state >information has been retrieved. This is not the case for s390, where >halted is an indication that the vCPU is stopped, meaning its not >available to the guest operating system until it has been restarted. > > The following patches help to alleviate the issues: > > Patch 1/3: > Adds architecture specific data to the QMP CpuInfo type, exposing > the existing s390 cpu-state in QMP. The cpu-state is a representation > more adequate than the ambiguous 'halted' condition. > > Patch 2/3: > Adds a new QMP function query-cpus-fast, which will only retrieve > vCPU information that can be obtained without interrupting the > vCPUs of a running VM. It introduces a new return type CpuInfoFast > with the subset of fields meeting this condition. Specifically, the > halted state is not part of CpuInfoFast. QMP clients like libvirt > are encouraged to switch to the new API for vCPU information. > > Patch 3/3: > Adds the s390-specific cpu state to CpuInfoFast, allowing management > apps to find out whether a vCPU is in the stopped state. This extension > leads to a partial duplication of field definitions from CpuInfo > to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and > eventually removed. How shall we proceed with this series? Patch 3 depends upon patch 1, so I think it makes sense to merge this in one go. I can give my R-b on patch 1 and Someone(tm) can merge this, or I can take the whole series through the s390 tree (with some further reviews/acks on patches 2/3).
[Qemu-devel] [PATCHv2 0/3] add query-cpu-fast and related s390 changes
This series consolidates patches around a performance issue caused by the usage of QMP query-cpus. A performance issue was found in an OpenStack environment, where ceilometer was collecting domain statistics with libvirt. The domain statistics reported by libvirt include the vCPU halted state, which in turn is retrieved with QMP query-cpus. This causes two issues: 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl to find out whether a vCPU was halted. This is not the case for s390 but query-cpus is always causing the vCPU to exit the VM. 2. Semantics: on x86 and other architectures, halted is a highly transient state, which is likely to have already changed shortly after the state information has been retrieved. This is not the case for s390, where halted is an indication that the vCPU is stopped, meaning its not available to the guest operating system until it has been restarted. The following patches help to alleviate the issues: Patch 1/3: Adds architecture specific data to the QMP CpuInfo type, exposing the existing s390 cpu-state in QMP. The cpu-state is a representation more adequate than the ambiguous 'halted' condition. Patch 2/3: Adds a new QMP function query-cpus-fast, which will only retrieve vCPU information that can be obtained without interrupting the vCPUs of a running VM. It introduces a new return type CpuInfoFast with the subset of fields meeting this condition. Specifically, the halted state is not part of CpuInfoFast. QMP clients like libvirt are encouraged to switch to the new API for vCPU information. Patch 3/3: Adds the s390-specific cpu state to CpuInfoFast, allowing management apps to find out whether a vCPU is in the stopped state. This extension leads to a partial duplication of field definitions from CpuInfo to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and eventually removed. Series v1 -> v2: Patch 2/3: o Changed formatting of hmp info cpus_fast to match that of info cpus. This makes it easier for clients to switch to the fast call. Patch 3/3: o Same formatting change for info cpus_fast as in 2/3, only for s390-specific cpu state. Luiz Capitulino (1): qmp: add query-cpus-fast Viktor Mihajlovski (2): qmp: expose s390-specific CPU info qmp: add architecture specific cpu data for query-cpus-fast cpus.c | 54 hmp-commands-info.hx | 14 ++ hmp.c | 28 +++ hmp.h | 1 + hw/intc/s390_flic.c| 4 +- hw/s390x/s390-virtio-ccw.c | 2 +- qapi-schema.json | 121 - target/s390x/cpu.c | 24 - target/s390x/cpu.h | 7 +-- target/s390x/kvm.c | 8 +-- target/s390x/sigp.c| 38 +++--- 11 files changed, 257 insertions(+), 44 deletions(-) -- 1.9.1