| Message ID | 1518542328-25741-4-git-send-email-mihajlov@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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.
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.
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.
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).
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 --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:
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(-)