| Message ID | 20180208111732.09869f84@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] qmp: add query-cpus-fast | expand |
On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote: [...] > The "halted" field is somewhat controversial. On the one hand, > it offers a convenient way to know if a guest CPU is idle or > running. On the other hand, it's a field that can change many > times a second. In fact, the halted state can change even > before query-cpus-fast has returned. This makes one wonder if > this field should be dropped all together. Having the "halted" > field as optional gives a better option for dropping it in > the future, since we can just stop returning it. I'd just drop it, unless we find a use case where it's really useful. Also, the code that sets/clears cpu->halted is target-specific, so I wouldn't be so sure that simply checking for !kvm_irqchip_in_kernel() is enough on all targets.
On 08.02.2018 21:33, Eduardo Habkost wrote: > On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote: > [...] >> The "halted" field is somewhat controversial. On the one hand, >> it offers a convenient way to know if a guest CPU is idle or >> running. On the other hand, it's a field that can change many >> times a second. In fact, the halted state can change even >> before query-cpus-fast has returned. This makes one wonder if >> this field should be dropped all together. Having the "halted" >> field as optional gives a better option for dropping it in >> the future, since we can just stop returning it. > > I'd just drop it, unless we find a use case where it's really > useful. > > Also, the code that sets/clears cpu->halted is target-specific, > so I wouldn't be so sure that simply checking for > !kvm_irqchip_in_kernel() is enough on all targets. > Right, the present patch effectively disables halted anyway (including s390). So it may be cleaner to just drop it right now. Assuming the presence of architecure-specific data, libvirt can derive a halted state (or an equivalent thereof) from query-cpus-fast returned information.
On Fri, 9 Feb 2018 08:56:19 +0100 Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > On 08.02.2018 21:33, Eduardo Habkost wrote: > > On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote: > > [...] > >> The "halted" field is somewhat controversial. On the one hand, > >> it offers a convenient way to know if a guest CPU is idle or > >> running. On the other hand, it's a field that can change many > >> times a second. In fact, the halted state can change even > >> before query-cpus-fast has returned. This makes one wonder if > >> this field should be dropped all together. Having the "halted" > >> field as optional gives a better option for dropping it in > >> the future, since we can just stop returning it. > > > > I'd just drop it, unless we find a use case where it's really > > useful. I don't think there's any, unless for debugging purposes. I'm keeping it mainly for s390. Viktor, libvirt is still using this field in s390, no? Dropping halted and having management software still using query-cpus because of halted would be a total failure of query-cpus-fast. > > Also, the code that sets/clears cpu->halted is target-specific, > > so I wouldn't be so sure that simply checking for > > !kvm_irqchip_in_kernel() is enough on all targets. I checked the code and had the impression it was enough, but I don't have experience with other archs. So, would be nice if other archs maintainers could review this. I'll try to ping them. > Right, the present patch effectively disables halted anyway (including > s390). No, it doesn't. It only disables halted for archs that require going to the kernel to get it. > So it may be cleaner to just drop it right now. > Assuming the presence of architecure-specific data, libvirt can derive a > halted state (or an equivalent thereof) from query-cpus-fast returned > information. This is a different proposal. You're proposing moving the halted state to a CPU-specific field. This is doable if that's what we want.
On 09.02.2018 14:49, Luiz Capitulino wrote: > On Fri, 9 Feb 2018 08:56:19 +0100 > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > >> On 08.02.2018 21:33, Eduardo Habkost wrote: >>> On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote: >>> [...] >>>> The "halted" field is somewhat controversial. On the one hand, >>>> it offers a convenient way to know if a guest CPU is idle or >>>> running. On the other hand, it's a field that can change many >>>> times a second. In fact, the halted state can change even >>>> before query-cpus-fast has returned. This makes one wonder if >>>> this field should be dropped all together. Having the "halted" >>>> field as optional gives a better option for dropping it in >>>> the future, since we can just stop returning it. >>> >>> I'd just drop it, unless we find a use case where it's really >>> useful. > > I don't think there's any, unless for debugging purposes. > > I'm keeping it mainly for s390. Viktor, libvirt is still using > this field in s390, no? see below > > Dropping halted and having management software still using query-cpus > because of halted would be a total failure of query-cpus-fast. > >>> Also, the code that sets/clears cpu->halted is target-specific, >>> so I wouldn't be so sure that simply checking for >>> !kvm_irqchip_in_kernel() is enough on all targets. > > I checked the code and had the impression it was enough, but > I don't have experience with other archs. So, would be nice > if other archs maintainers could review this. I'll try to ping them. > >> Right, the present patch effectively disables halted anyway (including >> s390). > > No, it doesn't. It only disables halted for archs that require going > to the kernel to get it.> >> So it may be cleaner to just drop it right now. >> Assuming the presence of architecure-specific data, libvirt can derive a >> halted state (or an equivalent thereof) from query-cpus-fast returned >> information. > > This is a different proposal. You're proposing moving the halted state > to a CPU-specific field. This is doable if that's what we want. > In order to use query-cpus-fast, libvirt has to change code anyway. Since libvirt is only providing cpu.n.halted for s390, and this value can be derived from the cpu-state, there's no need for QEMU to return halted in query-cpus-fast any more.
On Fri, Feb 09, 2018 at 08:49:49AM -0500, Luiz Capitulino wrote: > On Fri, 9 Feb 2018 08:56:19 +0100 > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > > > On 08.02.2018 21:33, Eduardo Habkost wrote: > > > On Thu, Feb 08, 2018 at 11:17:32AM -0500, Luiz Capitulino wrote: > > > [...] > > >> The "halted" field is somewhat controversial. On the one hand, > > >> it offers a convenient way to know if a guest CPU is idle or > > >> running. On the other hand, it's a field that can change many > > >> times a second. In fact, the halted state can change even > > >> before query-cpus-fast has returned. This makes one wonder if > > >> this field should be dropped all together. Having the "halted" > > >> field as optional gives a better option for dropping it in > > >> the future, since we can just stop returning it. > > > > > > I'd just drop it, unless we find a use case where it's really > > > useful. > > I don't think there's any, unless for debugging purposes. > > I'm keeping it mainly for s390. Viktor, libvirt is still using > this field in s390, no? > > Dropping halted and having management software still using query-cpus > because of halted would be a total failure of query-cpus-fast. If I understood correctly, the CpuInfoS390::cpu_state field added by Viktor in another patch[1] would replace "halted" for the s390 case. I'm assuming QEMU will be able to return that field without interrupting the VCPUs. Viktor, is that correct? [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html > > > > Also, the code that sets/clears cpu->halted is target-specific, > > > so I wouldn't be so sure that simply checking for > > > !kvm_irqchip_in_kernel() is enough on all targets. > > I checked the code and had the impression it was enough, but > I don't have experience with other archs. So, would be nice > if other archs maintainers could review this. I'll try to ping them. I think we need to take a step back and rethink: 1) What the field is supposed to mean? The semantics of "halted" are completely unclear. What exactly we want to communicate to libvirt/management? 2) On which cases the information (whatever it means) is really useful/important? If you are excluding cases with in-kernel irqchip, you are already excluding most users. > > > Right, the present patch effectively disables halted anyway (including > > s390). > > No, it doesn't. It only disables halted for archs that require going > to the kernel to get it. It disables it for all architectures that implement in-kernel irqchip: x86, arm, s390. The only existing user of "halted" is s390-specific code in libvirt, and your patch won't return it on s390, so nobody seems to benefit from it. > > > So it may be cleaner to just drop it right now. > > Assuming the presence of architecure-specific data, libvirt can derive a > > halted state (or an equivalent thereof) from query-cpus-fast returned > > information. > > This is a different proposal. You're proposing moving the halted state > to a CPU-specific field. This is doable if that's what we want. I think it's a valid approach to return a target-specific field first, and later try to come up with a generic (and clearly defined) abstraction to represent the same information (either inside QEMU or inside libvirt).
On 09.02.2018 15:27, Eduardo Habkost wrote: [...] >> I'm keeping it mainly for s390. Viktor, libvirt is still using >> this field in s390, no? >> >> Dropping halted and having management software still using query-cpus >> because of halted would be a total failure of query-cpus-fast. > > If I understood correctly, the CpuInfoS390::cpu_state field added > by Viktor in another patch[1] would replace "halted" for the s390 > case. Right, CPUState.halted is derived from CPUS390XState.cpu_state on s390: A cpu_state of CPU_STATE_STOPPED or CPU_STATE_CHECK_STOPPED results in halted = true. This derivation can be done by libvirt for s390. > > I'm assuming QEMU will be able to return that field without > interrupting the VCPUs. Viktor, is that correct? > > [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html > That's correct. >> >>>> Also, the code that sets/clears cpu->halted is target-specific, >>>> so I wouldn't be so sure that simply checking for >>>> !kvm_irqchip_in_kernel() is enough on all targets. >> >> I checked the code and had the impression it was enough, but >> I don't have experience with other archs. So, would be nice >> if other archs maintainers could review this. I'll try to ping them. > > I think we need to take a step back and rethink: > > 1) What the field is supposed to mean? The semantics of "halted" > are completely unclear. What exactly we want to communicate > to libvirt/management? > 2) On which cases the information (whatever it means) is really > useful/important? If you are excluding cases with in-kernel > irqchip, you are already excluding most users. > > Given that nobody (including myself) sees a need for halted we can remove it for the fast version of query-cpus without surprising anyone. [...]
On Fri, 9 Feb 2018 15:50:00 +0100 Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > On 09.02.2018 15:27, Eduardo Habkost wrote: > [...] > >> I'm keeping it mainly for s390. Viktor, libvirt is still using > >> this field in s390, no? > >> > >> Dropping halted and having management software still using query-cpus > >> because of halted would be a total failure of query-cpus-fast. > > > > If I understood correctly, the CpuInfoS390::cpu_state field added > > by Viktor in another patch[1] would replace "halted" for the s390 > > case. > Right, CPUState.halted is derived from CPUS390XState.cpu_state on s390: > A cpu_state of CPU_STATE_STOPPED or CPU_STATE_CHECK_STOPPED results in > halted = true. This derivation can be done by libvirt for s390. > > > > I'm assuming QEMU will be able to return that field without > > interrupting the VCPUs. Viktor, is that correct? > > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02032.html > > > That's correct. > >> > >>>> Also, the code that sets/clears cpu->halted is target-specific, > >>>> so I wouldn't be so sure that simply checking for > >>>> !kvm_irqchip_in_kernel() is enough on all targets. > >> > >> I checked the code and had the impression it was enough, but > >> I don't have experience with other archs. So, would be nice > >> if other archs maintainers could review this. I'll try to ping them. > > > > I think we need to take a step back and rethink: > > > > 1) What the field is supposed to mean? The semantics of "halted" > > are completely unclear. What exactly we want to communicate > > to libvirt/management? > > 2) On which cases the information (whatever it means) is really > > useful/important? If you are excluding cases with in-kernel > > irqchip, you are already excluding most users. > > > > > Given that nobody (including myself) sees a need for halted we can > remove it for the fast version of query-cpus without surprising anyone. > [...] My conclusion too, I'll drop it.
diff --git a/cpus.c b/cpus.c index 182caf764e..905b1f4d79 100644 --- a/cpus.c +++ b/cpus.c @@ -2150,6 +2150,50 @@ CpuInfoList *qmp_query_cpus(Error **errp) return head; } +/* + * fast means: we NEVER interrupt vCPU threads to retrieve + * information from KVM. + */ +CpuInfoFastList *qmp_query_cpus_fast(Error **errp) +{ + MachineState *ms = MACHINE(qdev_get_machine()); + MachineClass *mc = MACHINE_GET_CLASS(ms); + CpuInfoFastList *head = NULL, *cur_item = NULL; + CPUState *cpu; + + CPU_FOREACH(cpu) { + CpuInfoFastList *info = g_malloc0(sizeof(*info)); + info->value = g_malloc0(sizeof(*info->value)); + + info->value->cpu_index = cpu->cpu_index; + info->value->qom_path = object_get_canonical_path(OBJECT(cpu)); + info->value->thread_id = cpu->thread_id; + + info->value->has_props = !!mc->cpu_index_to_instance_props; + if (info->value->has_props) { + CpuInstanceProperties *props; + props = g_malloc0(sizeof(*props)); + *props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index); + info->value->props = props; + } + + /* if in kernel irqchip is used, we don't have 'halted' */ + info->value->has_halted = !kvm_irqchip_in_kernel(); + if (info->value->has_halted) { + info->value->halted = cpu->halted; + } + + if (!cur_item) { + head = cur_item = info; + } else { + cur_item->next = info; + cur_item = info; + } + } + + return head; +} + void qmp_memsave(int64_t addr, int64_t size, const char *filename, bool has_cpu, int64_t cpu_index, Error **errp) { diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index ad590a4ffb..16ac60285f 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -157,6 +157,20 @@ STEXI @item info cpus @findex info cpus Show infos for each CPU. +ETEXI + + { + .name = "cpus_fast", + .args_type = "", + .params = "", + .help = "show information for each CPU without interrupting them", + .cmd = hmp_info_cpus_fast, + }, + +STEXI +@item info cpus_fast +@findex info cpus_fast +Show infos for each CPU without performance penalty. ETEXI { diff --git a/hmp.c b/hmp.c index b3de32d219..90717c13b2 100644 --- a/hmp.c +++ b/hmp.c @@ -404,6 +404,30 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) qapi_free_CpuInfoList(cpu_list); } +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict) +{ + CpuInfoFastList *head, *cpu; + TargetInfo *target; + + target = qmp_query_target(NULL); + monitor_printf(mon, "CPU architecture is '%s'\n\n", target->arch); + qapi_free_TargetInfo(target); + + head = qmp_query_cpus_fast(NULL); + + for (cpu = head; cpu; cpu = cpu->next) { + monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index); + monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id); + if (cpu->value->has_halted) { + monitor_printf(mon, " halted=%d\n", cpu->value->halted); + } + monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path); + monitor_printf(mon, "\n"); + } + + qapi_free_CpuInfoFastList(head); +} + static void print_block_info(Monitor *mon, BlockInfo *info, BlockDeviceInfo *inserted, bool verbose) { diff --git a/hmp.h b/hmp.h index 536cb91caa..f4ceba5cc8 100644 --- a/hmp.h +++ b/hmp.h @@ -31,6 +31,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict); void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict); void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict); void hmp_info_cpus(Monitor *mon, const QDict *qdict); +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict); void hmp_info_block(Monitor *mon, const QDict *qdict); void hmp_info_blockstats(Monitor *mon, const QDict *qdict); void hmp_info_vnc(Monitor *mon, const QDict *qdict); diff --git a/qapi-schema.json b/qapi-schema.json index 5c06745c79..9bc37ea604 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -526,6 +526,12 @@ # # Returns a list of information about each virtual CPU. # +# This command causes vCPU threads to exit to userspace, which causes +# an small interruption guest CPU execution. This will have a negative +# impact on realtime guests and other latency sensitive guest workloads. +# It is recommended to use @query-cpus-fast instead of this command to +# avoid the vCPU interruption. +# # Returns: a list of @CpuInfo for each virtual CPU # # Since: 0.14.0 @@ -558,6 +564,77 @@ ## { 'command': 'query-cpus', 'returns': ['CpuInfo'] } +## +# @CpuInfoFast: +# +# Information about a virtual CPU +# +# @cpu-index: index of the virtual CPU +# +# @halted: true if the virtual CPU is in the halt state. Halt usually refers +# to a processor specific low power mode. This field is optional, +# it is only present if the halted state can be retrieved without +# a performance penalty +# +# @qom-path: path to the CPU object in the QOM tree +# +# @thread-id: ID of the underlying host thread +# +# @props: properties describing to which node/socket/core/thread +# virtual CPU belongs to, provided if supported by board +# +# Since: 2.12 +# +# Notes: @halted is a transient state that changes frequently. By the time the +# data is sent to the client, the guest may no longer be halted. +## +{ 'struct': 'CpuInfoFast', + 'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str', + 'thread-id': 'int', '*props': 'CpuInstanceProperties' } } + +## +# @query-cpus-fast: +# +# Returns information about all virtual CPUs. This command does not +# incur a performance penalty and should be used in production +# instead of query-cpus. +# +# Returns: list of @CpuInfoFast +# +# Notes: The CPU architecture name is not returned by query-cpus-fast. +# Use query-target to retrieve that information. +# +# Since: 2.12 +# +# Example: +# +# -> { "execute": "query-cpus-fast" } +# <- { "return": [ +# { +# "thread-id": 25627, +# "props": { +# "core-id": 0, +# "thread-id": 0, +# "socket-id": 0 +# }, +# "qom-path": "/machine/unattached/device[0]", +# "cpu-index": 0 +# }, +# { +# "thread-id": 25628, +# "props": { +# "core-id": 0, +# "thread-id": 0, +# "socket-id": 1 +# }, +# "qom-path": "/machine/unattached/device[2]", +# "cpu-index": 1 +# } +# ] +# } +## +{ 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] } + ## # @IOThreadInfo: #
The query-cpus command has an extremely serious side effect: it always interrupts all running vCPUs so that they can run ioctl calls. This can cause a huge performance degradation for some workloads. And most of the information retrieved by the ioctl calls are not even used by query-cpus. This commit introduces a replacement for query-cpus called query-cpus-fast, which has the following features: o Never interrupt vCPUs threads. query-cpus-fast only returns vCPU information maintained by QEMU itself, which should be sufficient for most management software needs o Make "halted" field optional: we only return it if the halted state is maintained by QEMU. But this also gives the option of dropping the field in the future (see below) o Drop irrelevant fields such as "current", "pc" and "arch" o Rename some fields for better clarification & proper naming standard The "halted" field is somewhat controversial. On the one hand, it offers a convenient way to know if a guest CPU is idle or running. On the other hand, it's a field that can change many times a second. In fact, the halted state can change even before query-cpus-fast has returned. This makes one wonder if this field should be dropped all together. Having the "halted" field as optional gives a better option for dropping it in the future, since we can just stop returning it. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- o v2 - Many English and naming corrections by Eric - Adopt query-cpus text from Daniel - squash query-cpus text change into this patch cpus.c | 44 ++++++++++++++++++++++++++++++ hmp-commands-info.hx | 14 ++++++++++ hmp.c | 24 ++++++++++++++++ hmp.h | 1 + qapi-schema.json | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 160 insertions(+)