On 16.02.2018 15:03, Eric Blake wrote: > On 02/16/2018 06:04 AM, Viktor Mihajlovski wrote: >> From: Luiz Capitulino <lcapitul...@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 <lcapitul...@redhat.com> >> Signed-off-by: Viktor Mihajlovski <mihaj...@linux.vnet.ibm.com> >> Reviewed-by: Cornelia Huck <coh...@redhat.com> >> Acked-by: Eric Blake <ebl...@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...
-- Regards, Viktor Mihajlovski