| Message ID | 1518782691-1232-3-git-send-email-mihajlov@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/16/2018 06:04 AM, Viktor Mihajlovski wrote: > From: Luiz Capitulino <lcapitulino@redhat.com> > > 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 Drop "halted" field as it can not be retrieved in a fast > way on most architectures > > o Drop irrelevant fields such as "current", "pc" and "arch" > > o Rename some fields for better clarification & proper naming > standard > > Further, the HMP "info cpus" implementation is changed to > use the new QMP interface to avoid the side effects caused > by query-cpus. This means that only a reduced subset of cpu > information will be reported, which is acceptable as > the content of "info cpus" is not documented or guaranteed > to be stable. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Acked-by: Eric Blake <eblake@redhat.com> The HMP changes are non-trivial compared to v3, so I might have dropped all R-b and Acked-by to ensure they are looked at again. In fact,... > --- > cpus.c | 38 ++++++++++++++++++++++++++++++ > hmp.c | 46 +++++-------------------------------- > qapi-schema.json | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 114 insertions(+), 40 deletions(-) > > +++ b/hmp.c > @@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict) > > void hmp_info_cpus(Monitor *mon, const QDict *qdict) > { > - CpuInfoList *cpu_list, *cpu; > + CpuInfoFastList *head, *cpu; > > - cpu_list = qmp_query_cpus(NULL); > + head = qmp_query_cpus_fast(NULL); > > - for (cpu = cpu_list; cpu; cpu = cpu->next) { > - int active = ' '; > - > - if (cpu->value->CPU == monitor_get_cpu_index()) { > - active = '*'; > - } The old code was doing multiple things - it was telling you the current HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using HMP, knowing which cpu is the active target for future HMP commands that depend on the active target, this bit of information is important), as well as... > - monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU); > - > - switch (cpu->value->arch) { > - case CPU_INFO_ARCH_X86: > - monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc); > - break; ...inundating you with the arch-specific slow stuff. I'm in agreement that dropping the slow stuff is okay, but... > + for (cpu = head; cpu; cpu = cpu->next) { > + monitor_printf(mon, " CPU #%" PRId64 ":", cpu->value->cpu_index); ...unilaterally dropping the '*' active indicator, which has no bearing on the QMP changes, and thus is not an appropriate change for this patch. I've now gone through the entire patch (and not just the UI), so on the next revision, I'll be prepared to give a full R-b, rather than just Acked-by. But I do think this means we need a v5.
On 16.02.2018 15:03, Eric Blake wrote: > On 02/16/2018 06:04 AM, Viktor Mihajlovski wrote: >> From: Luiz Capitulino <lcapitulino@redhat.com> >> >> 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 Drop "halted" field as it can not be retrieved in a fast >> way on most architectures >> >> o Drop irrelevant fields such as "current", "pc" and "arch" >> >> o Rename some fields for better clarification & proper naming >> standard >> >> Further, the HMP "info cpus" implementation is changed to >> use the new QMP interface to avoid the side effects caused >> by query-cpus. This means that only a reduced subset of cpu >> information will be reported, which is acceptable as >> the content of "info cpus" is not documented or guaranteed >> to be stable. >> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >> Acked-by: Eric Blake <eblake@redhat.com> > > The HMP changes are non-trivial compared to v3, so I might have dropped > all R-b and Acked-by to ensure they are looked at again. > > In fact,... > >> --- >> cpus.c | 38 ++++++++++++++++++++++++++++++ >> hmp.c | 46 +++++-------------------------------- >> qapi-schema.json | 70 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 114 insertions(+), 40 deletions(-) >> > >> +++ b/hmp.c >> @@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon, >> const QDict *qdict) >> void hmp_info_cpus(Monitor *mon, const QDict *qdict) >> { >> - CpuInfoList *cpu_list, *cpu; >> + CpuInfoFastList *head, *cpu; >> - cpu_list = qmp_query_cpus(NULL); >> + head = qmp_query_cpus_fast(NULL); >> - for (cpu = cpu_list; cpu; cpu = cpu->next) { >> - int active = ' '; >> - >> - if (cpu->value->CPU == monitor_get_cpu_index()) { >> - active = '*'; >> - } > > The old code was doing multiple things - it was telling you the current > HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using > HMP, knowing which cpu is the active target for future HMP commands that > depend on the active target, this bit of information is important), as > well as... > >> - monitor_printf(mon, "%c CPU #%" PRId64 ":", active, >> cpu->value->CPU); >> - >> - switch (cpu->value->arch) { >> - case CPU_INFO_ARCH_X86: >> - monitor_printf(mon, " pc=0x%016" PRIx64, >> cpu->value->u.x86.pc); >> - break; > > ...inundating you with the arch-specific slow stuff. I'm in agreement > that dropping the slow stuff is okay, but... > >> + for (cpu = head; cpu; cpu = cpu->next) { >> + monitor_printf(mon, " CPU #%" PRId64 ":", >> cpu->value->cpu_index); > > ...unilaterally dropping the '*' active indicator, which has no bearing > on the QMP changes, and thus is not an appropriate change for this patch> > I've now gone through the entire patch (and not just the UI), so on the > next revision, I'll be prepared to give a full R-b, rather than just > Acked-by. But I do think this means we need a v5. > Thanks for the patience. I'll respin with the r/a-b's on patch 2/4 removed, but want to verify first that I can get the active cpu without triggering cpu_synchronize_state via monitor_get_cpu_index...
On 02/16/2018 08:49 AM, Viktor Mihajlovski wrote: >> >> The HMP changes are non-trivial compared to v3, so I might have dropped >> all R-b and Acked-by to ensure they are looked at again. >> >> In fact,... >> >>> - >>> - if (cpu->value->CPU == monitor_get_cpu_index()) { >>> - active = '*'; >>> - } >> >> The old code was doing multiple things - it was telling you the current >> HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using >> HMP, knowing which cpu is the active target for future HMP commands that >> depend on the active target, this bit of information is important), >> > Thanks for the patience. I'll respin with the r/a-b's on patch 2/4 > removed, but want to verify first that I can get the active cpu without > triggering cpu_synchronize_state via monitor_get_cpu_index... What does it matter? HMP is not the time-critical interface like QMP, so even if it has to synchronize things, you're no worse off than you were before when using HMP.
* Eric Blake (eblake@redhat.com) wrote: > On 02/16/2018 08:49 AM, Viktor Mihajlovski wrote: > > > > > > > The HMP changes are non-trivial compared to v3, so I might have dropped > > > all R-b and Acked-by to ensure they are looked at again. > > > > > > In fact,... > > > > > > > > - > > > > - if (cpu->value->CPU == monitor_get_cpu_index()) { > > > > - active = '*'; > > > > - } > > > > > > The old code was doing multiple things - it was telling you the current > > > HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using > > > HMP, knowing which cpu is the active target for future HMP commands that > > > depend on the active target, this bit of information is important), > > > > > > Thanks for the patience. I'll respin with the r/a-b's on patch 2/4 > > removed, but want to verify first that I can get the active cpu without > > triggering cpu_synchronize_state via monitor_get_cpu_index... > > What does it matter? HMP is not the time-critical interface like QMP, so > even if it has to synchronize things, you're no worse off than you were > before when using HMP. I guess it's not obvious that mon_get_cpu_index has that side effect; although I can see why it's there since it guarantees correctness of all the HMP data with one call; a comment might be nice to point it out. But yes, I agree the performance cost there doesn't worry me for HMP. Dave > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/cpus.c b/cpus.c index 6006931..6df6660 100644 --- a/cpus.c +++ b/cpus.c @@ -2156,6 +2156,44 @@ 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 (!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.c b/hmp.c index 7870d6a..28b1070 100644 --- a/hmp.c +++ b/hmp.c @@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict) void hmp_info_cpus(Monitor *mon, const QDict *qdict) { - CpuInfoList *cpu_list, *cpu; + CpuInfoFastList *head, *cpu; - cpu_list = qmp_query_cpus(NULL); + head = qmp_query_cpus_fast(NULL); - for (cpu = cpu_list; cpu; cpu = cpu->next) { - int active = ' '; - - if (cpu->value->CPU == monitor_get_cpu_index()) { - active = '*'; - } - - monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU); - - switch (cpu->value->arch) { - case CPU_INFO_ARCH_X86: - monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc); - break; - case CPU_INFO_ARCH_PPC: - monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip); - break; - case CPU_INFO_ARCH_SPARC: - monitor_printf(mon, " pc=0x%016" PRIx64, - cpu->value->u.q_sparc.pc); - monitor_printf(mon, " npc=0x%016" PRIx64, - cpu->value->u.q_sparc.npc); - break; - case CPU_INFO_ARCH_MIPS: - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips.PC); - break; - case CPU_INFO_ARCH_TRICORE: - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC); - break; - default: - break; - } - - if (cpu->value->halted) { - monitor_printf(mon, " (halted)"); - } - - monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id); + for (cpu = head; cpu; cpu = cpu->next) { + monitor_printf(mon, " CPU #%" PRId64 ":", cpu->value->cpu_index); + monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id); } - qapi_free_CpuInfoList(cpu_list); + qapi_free_CpuInfoFastList(head); } static void print_block_info(Monitor *mon, BlockInfo *info, diff --git a/qapi-schema.json b/qapi-schema.json index 94d560e..815f072 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -552,6 +552,12 @@ # # Returns a list of information about each virtual CPU. # +# This command causes vCPU threads to exit to userspace, which causes +# a small interruption to 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 @@ -585,6 +591,70 @@ { 'command': 'query-cpus', 'returns': ['CpuInfo'] } ## +# @CpuInfoFast: +# +# Information about a virtual CPU +# +# @cpu-index: index of the virtual CPU +# +# @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 +# +## +{ 'struct': 'CpuInfoFast', + 'data': {'cpu-index': 'int', '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: # # Information about an iothread