Re: [qemu-s390x] [PATCHv4 2/4] qmp: add query-cpus-fast

[Viktor Mihajlovski]

On 16.02.2018 15:03, Eric Blake wrote:
> On 02/16/2018 06:04 AM, Viktor Mihajlovski wrote:
>> From: Luiz Capitulino <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Viktor Mihajlovski <address@hidden>
>> Reviewed-by: Cornelia Huck <address@hidden>
>> Acked-by: Eric Blake <address@hidden>
> 
> 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