diff mbox

[PATCHv2,3/3] qmp: add architecture specific cpu data for query-cpus-fast

Message ID 1518542328-25741-4-git-send-email-mihajlov@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Viktor Mihajlovski Feb. 13, 2018, 5:18 p.m. UTC
The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfoFast union with architecture
specific data and an implementation for s390.

Return data looks like this:
 [
   {"thread-id":64301,"props":{"core-id":0},
    "arch":"s390","cpu-state":"operating",
    "qom-path":"/machine/unattached/device[0]","cpu-index":0},
   {"thread-id":64302,"props":{"core-id":1},
    "arch":"s390","cpu-state":"operating",
    "qom-path":"/machine/unattached/device[1]","cpu-index":1}
]

Currently there's a certain amount of duplication between
the definitions of CpuInfo and CpuInfoFast, both in the
base and variable areas, since there are data fields common
to the slow and fast variants.

A suggestion was made on the mailing list to enhance the QAPI
code generation to support two layers of unions. This would
allow to specify the common fields once and avoid the duplication
in the leaf unions.

On the other hand, the slow query-cpus should be deprecated
along with the slow CpuInfo type and eventually be removed.
Assuming that new architectures will not be added at high
rates, we could live with the duplication for the time being.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 cpus.c           | 10 ++++++++++
 hmp.c            | 10 ++++++++++
 qapi-schema.json | 35 +++++++++++++++++++++++++++++------
 3 files changed, 49 insertions(+), 6 deletions(-)

Comments

Cornelia Huck Feb. 14, 2018, 10:15 a.m. UTC | #1
On Tue, 13 Feb 2018 18:18:48 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> The s390 CPU state can be retrieved without interrupting the
> VM execution. Extendend the CpuInfoFast union with architecture
> specific data and an implementation for s390.
> 
> Return data looks like this:
>  [
>    {"thread-id":64301,"props":{"core-id":0},
>     "arch":"s390","cpu-state":"operating",
>     "qom-path":"/machine/unattached/device[0]","cpu-index":0},
>    {"thread-id":64302,"props":{"core-id":1},
>     "arch":"s390","cpu-state":"operating",
>     "qom-path":"/machine/unattached/device[1]","cpu-index":1}
> ]
> 
> Currently there's a certain amount of duplication between
> the definitions of CpuInfo and CpuInfoFast, both in the
> base and variable areas, since there are data fields common
> to the slow and fast variants.
> 
> A suggestion was made on the mailing list to enhance the QAPI
> code generation to support two layers of unions. This would
> allow to specify the common fields once and avoid the duplication
> in the leaf unions.
> 
> On the other hand, the slow query-cpus should be deprecated
> along with the slow CpuInfo type and eventually be removed.
> Assuming that new architectures will not be added at high
> rates, we could live with the duplication for the time being.

What would be a realistic timeframe for deprecation/removal of
query-cpus, considering the libvirt usage? Are we aware of other users?

> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  cpus.c           | 10 ++++++++++
>  hmp.c            | 10 ++++++++++
>  qapi-schema.json | 35 +++++++++++++++++++++++++++++------
>  3 files changed, 49 insertions(+), 6 deletions(-)

Patch looks good to me.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Eric Blake Feb. 14, 2018, 3:15 p.m. UTC | #2
On 02/14/2018 04:15 AM, Cornelia Huck wrote:
> On Tue, 13 Feb 2018 18:18:48 +0100
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> 

>> A suggestion was made on the mailing list to enhance the QAPI
>> code generation to support two layers of unions. This would
>> allow to specify the common fields once and avoid the duplication
>> in the leaf unions.
>>
>> On the other hand, the slow query-cpus should be deprecated
>> along with the slow CpuInfo type and eventually be removed.
>> Assuming that new architectures will not be added at high
>> rates, we could live with the duplication for the time being.
> 
> What would be a realistic timeframe for deprecation/removal of
> query-cpus, considering the libvirt usage? Are we aware of other users?

Well, if we want to start the clock ticking, this series also needs to 
touch qemu-doc.texi to start the deprecation clock, then we go at least 
two more releases with support for both old and new commands.
Cornelia Huck Feb. 14, 2018, 3:27 p.m. UTC | #3
On Wed, 14 Feb 2018 09:15:23 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 02/14/2018 04:15 AM, Cornelia Huck wrote:
> > On Tue, 13 Feb 2018 18:18:48 +0100
> > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> >   
> 
> >> A suggestion was made on the mailing list to enhance the QAPI
> >> code generation to support two layers of unions. This would
> >> allow to specify the common fields once and avoid the duplication
> >> in the leaf unions.
> >>
> >> On the other hand, the slow query-cpus should be deprecated
> >> along with the slow CpuInfo type and eventually be removed.
> >> Assuming that new architectures will not be added at high
> >> rates, we could live with the duplication for the time being.  
> > 
> > What would be a realistic timeframe for deprecation/removal of
> > query-cpus, considering the libvirt usage? Are we aware of other users?  
> 
> Well, if we want to start the clock ticking, this series also needs to 
> touch qemu-doc.texi to start the deprecation clock, then we go at least 
> two more releases with support for both old and new commands.
> 

I'd like to know first what libvirt plans to do -- no sense in starting
deprecation if we're still stuck with it for a while.
Viktor Mihajlovski Feb. 14, 2018, 3:50 p.m. UTC | #4
On 14.02.2018 16:27, Cornelia Huck wrote:
> On Wed, 14 Feb 2018 09:15:23 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
>> On 02/14/2018 04:15 AM, Cornelia Huck wrote:
>>> On Tue, 13 Feb 2018 18:18:48 +0100
>>> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
>>>   
>>
>>>> A suggestion was made on the mailing list to enhance the QAPI
>>>> code generation to support two layers of unions. This would
>>>> allow to specify the common fields once and avoid the duplication
>>>> in the leaf unions.
>>>>
>>>> On the other hand, the slow query-cpus should be deprecated
>>>> along with the slow CpuInfo type and eventually be removed.
>>>> Assuming that new architectures will not be added at high
>>>> rates, we could live with the duplication for the time being.  
>>>
>>> What would be a realistic timeframe for deprecation/removal of
>>> query-cpus, considering the libvirt usage? Are we aware of other users?  
>>
>> Well, if we want to start the clock ticking, this series also needs to 
>> touch qemu-doc.texi to start the deprecation clock, then we go at least 
>> two more releases with support for both old and new commands.
>>
> 
> I'd like to know first what libvirt plans to do -- no sense in starting
> deprecation if we're still stuck with it for a while.
> 
FWIW, I'm currently preparing libvirt patches to use query-cpus-fast if
available. I wouldn't see why it would take more than one libvirt
release to get them in.

The question might rather be, which combinations of qemu and libvirt are
considered useful. E.g., I didn't upgrade libvirt in a while on my test
system but am using bleeding edge QEMU. But that doesn't necessarily
resemble a valid setup.
Eric Blake Feb. 14, 2018, 4:07 p.m. UTC | #5
On 02/14/2018 09:50 AM, Viktor Mihajlovski wrote:

>> I'd like to know first what libvirt plans to do -- no sense in starting
>> deprecation if we're still stuck with it for a while.
>>
> FWIW, I'm currently preparing libvirt patches to use query-cpus-fast if
> available. I wouldn't see why it would take more than one libvirt
> release to get them in.

And libvirt releases more frequently, so that's probably not a limiting 
factor for deprecating it now in qemu.

> 
> The question might rather be, which combinations of qemu and libvirt are
> considered useful. E.g., I didn't upgrade libvirt in a while on my test
> system but am using bleeding edge QEMU. But that doesn't necessarily
> resemble a valid setup.

In general, new libvirt and old qemu is supported (libvirt tries hard to 
make sure it can manage older images gracefully, for as long as older 
qemu is still supported by a distro), but old libvirt and new qemu is 
unsupported (you often get lucky where it works for a release or two, 
because qemu tries hard not to break back-compat without proper 
deprecation periods, but there may be features that you NEED a newer 
libvirt to properly drive, and this is one of those cases).
Eric Blake Feb. 14, 2018, 7:56 p.m. UTC | #6
On 02/13/2018 11:18 AM, Viktor Mihajlovski wrote:
> The s390 CPU state can be retrieved without interrupting the
> VM execution. Extendend the CpuInfoFast union with architecture

s/Extendend/Extend/

> specific data and an implementation for s390.
> 
> Return data looks like this:
>   [
>     {"thread-id":64301,"props":{"core-id":0},
>      "arch":"s390","cpu-state":"operating",
>      "qom-path":"/machine/unattached/device[0]","cpu-index":0},
>     {"thread-id":64302,"props":{"core-id":1},
>      "arch":"s390","cpu-state":"operating",
>      "qom-path":"/machine/unattached/device[1]","cpu-index":1}
> ]
> 
> Currently there's a certain amount of duplication between
> the definitions of CpuInfo and CpuInfoFast, both in the
> base and variable areas, since there are data fields common
> to the slow and fast variants.

Even with the difference in spelling that I pointed out in 2/3?

> 
> A suggestion was made on the mailing list to enhance the QAPI
> code generation to support two layers of unions. This would
> allow to specify the common fields once and avoid the duplication
> in the leaf unions.
> 
> On the other hand, the slow query-cpus should be deprecated
> along with the slow CpuInfo type and eventually be removed.
> Assuming that new architectures will not be added at high
> rates, we could live with the duplication for the time being.

Yes, this part is true.

> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>   cpus.c           | 10 ++++++++++
>   hmp.c            | 10 ++++++++++
>   qapi-schema.json | 35 +++++++++++++++++++++++++++++------
>   3 files changed, 49 insertions(+), 6 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -537,15 +537,26 @@
>     'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
>   
>   ##
> -# @CpuInfoS390:
> +# @CpuInfoS390Fast:
>   #
> -# Additional information about a virtual S390 CPU
> +# Additional information about a virtual S390 CPU which can be
> +# obtained without a performance penalty for a running VM
>   #
>   # @cpu-state: the virtual CPU's state
>   #
>   # Since: 2.12
>   ##
> -{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
> +{ 'struct': 'CpuInfoS390Fast', 'data': { 'cpu-state': 'CpuS390State' } }
> +
> +##
> +# @CpuInfoS390:
> +#
> +# Additional information about a virtual S390 CPU, potentially expensive
> +# to obtain
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'CpuInfoS390', 'base': 'CpuInfoS390Fast', 'data': { } }

This part works - but since you aren't adding any further fields, what 
is the point in having two type names?  Why not just document that 
CpuInfoS390 is always fast, whether used in CpuInfoFast or in the slower 
CpuInfo?  It's not like we get any additional compile-time safety by 
inventing a new type.

>   
>   ##
>   # @query-cpus:
> @@ -604,12 +615,24 @@
>   # @props: properties describing to which node/socket/core/thread
>   #         virtual CPU belongs to, provided if supported by board
>   #
> +# @arch: architecture of the cpu, which determines which additional fields
> +#        will be listed

Doing this leaves a stale comment in query-cpus-fast that the arch name 
is not provided; you'll need to delete that comment if this patch goes in.

> +#
>   # Since: 2.12
>   #
>   ##
> -{ 'struct': 'CpuInfoFast',
> -  'data': {'cpu-index': 'int', 'qom-path': 'str',
> -           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
> +{ 'union': 'CpuInfoFast',
> +  'base': {'cpu-index': 'int', 'qom-path': 'str',
> +           'thread-id': 'int', '*props': 'CpuInstanceProperties',
> +           'arch': 'CpuInfoArch' },
> +  'discriminator': 'arch',
> +  'data': { 'x86': 'CpuInfoOther',
> +            'sparc': 'CpuInfoOther',
> +            'ppc': 'CpuInfoOther',
> +            'mips': 'CpuInfoOther',
> +            'tricore': 'CpuInfoOther',
> +            's390': 'CpuInfoS390Fast',
> +            'other': 'CpuInfoOther' } }

CpuInfoOther is an empty class, so it works as a placeholder for all 
other arches without having the dichotomy like the 
CpuInfoS390/CpuInfoS390Fast split.

At any rate, whether or not you merge the two CpuInfoS390 type into one, 
converting CpuInfoFast into a union (and NOT trying to inherit between 
the slow type with different field names and the new fast type) seems 
reasonable, if libvirt really wants to keep using cpu-info-fast to learn 
whether S390 vcpus are halted.

>   ##
>   # @query-cpus-fast:

So, if you fix the stale comment here, a v3 patch can have
Acked-by: Eric Blake <eblake@redhat.com>

I also think your v3 series should touch qemu-doc.texi to start the 
deprecation timeframe.
diff mbox

Patch

diff --git a/cpus.c b/cpus.c
index 6df6660..af67826 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2166,6 +2166,10 @@  CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     CpuInfoFastList *head = NULL, *cur_item = NULL;
     CPUState *cpu;
+#if defined(TARGET_S390X)
+    S390CPU *s390_cpu;
+    CPUS390XState *env;
+#endif
 
     CPU_FOREACH(cpu) {
         CpuInfoFastList *info = g_malloc0(sizeof(*info));
@@ -2183,6 +2187,12 @@  CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
             info->value->props = props;
         }
 
+#if defined(TARGET_S390X)
+        s390_cpu = S390_CPU(cpu);
+        env = &s390_cpu->env;
+        info->value->arch = CPU_INFO_ARCH_S390;
+        info->value->u.s390.cpu_state = env->cpu_state;
+#endif
         if (!cur_item) {
             head = cur_item = info;
         } else {
diff --git a/hmp.c b/hmp.c
index 0bd3b3a..e27433e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -418,6 +418,16 @@  void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
 
     for (cpu = head; cpu; cpu = cpu->next) {
         monitor_printf(mon, "  CPU #%" PRId64 ":", cpu->value->cpu_index);
+
+        switch (cpu->value->arch) {
+        case CPU_INFO_ARCH_S390:
+            monitor_printf(mon, " state=%s",
+                           CpuS390State_str(cpu->value->u.s390.cpu_state));
+            break;
+        default:
+            break;
+        }
+
         monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
     }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index dc07729..b71f8cc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -537,15 +537,26 @@ 
   'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
 
 ##
-# @CpuInfoS390:
+# @CpuInfoS390Fast:
 #
-# Additional information about a virtual S390 CPU
+# Additional information about a virtual S390 CPU which can be
+# obtained without a performance penalty for a running VM
 #
 # @cpu-state: the virtual CPU's state
 #
 # Since: 2.12
 ##
-{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
+{ 'struct': 'CpuInfoS390Fast', 'data': { 'cpu-state': 'CpuS390State' } }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU, potentially expensive
+# to obtain
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'base': 'CpuInfoS390Fast', 'data': { } }
 
 ##
 # @query-cpus:
@@ -604,12 +615,24 @@ 
 # @props: properties describing to which node/socket/core/thread
 #         virtual CPU belongs to, provided if supported by board
 #
+# @arch: architecture of the cpu, which determines which additional fields
+#        will be listed
+#
 # Since: 2.12
 #
 ##
-{ 'struct': 'CpuInfoFast',
-  'data': {'cpu-index': 'int', 'qom-path': 'str',
-           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+{ 'union': 'CpuInfoFast',
+  'base': {'cpu-index': 'int', 'qom-path': 'str',
+           'thread-id': 'int', '*props': 'CpuInstanceProperties',
+           'arch': 'CpuInfoArch' },
+  'discriminator': 'arch',
+  'data': { 'x86': 'CpuInfoOther',
+            'sparc': 'CpuInfoOther',
+            'ppc': 'CpuInfoOther',
+            'mips': 'CpuInfoOther',
+            'tricore': 'CpuInfoOther',
+            's390': 'CpuInfoS390Fast',
+            'other': 'CpuInfoOther' } }
 
 ##
 # @query-cpus-fast: