Re: [Qemu-devel] [PATCH 1/2] qmp: add query-cpus-fast

[Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>]

On 08.02.2018 22:41, Eduardo Habkost wrote:
> On Thu, Feb 08, 2018 at 02:59:17PM -0600, Eric Blake wrote:
>> On 02/08/2018 01:59 PM, Eduardo Habkost wrote:
>>> On Wed, Feb 07, 2018 at 12:50:13PM -0500, Luiz Capitulino wrote:
>>>> The query-cpus command has an extremely serious side effect:
>>>> it always interrupt 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:
>>>>
>>
>>>> +# 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': 'CpuInfo2',
>>>> +  'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
>>>> +           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
>>>
>>> This will require duplicating struct fields every time we add a
>>> new field to query-cpus-fast (e.g. how would VIktor's
>>> CpuInfoS390State patch[1] look like if rebased on top of yours?).
>>>
>>> One way to avoid that is to use CpuInfo for both, and make all
>>> "slow"  fields optional.  Another option is to use QAPI
>>> inheritance, but it could be a little complicated if unions are
>>> involved?
>>
>> Inheritance is better than optional fields for the sake of introspection
>> learning which fields to expect.
>>
>> Put the common fields to both interfaces in the base class, then have the
>> slower (older) CpuInfo class extend the base class to add the additional
>> fields.
>>
>> Unions should be able to inherit just fine from structs (after all, a flat
>> union requires a struct base); but if we need two layers of unions, we'll
>> need to enhance QAPI code generation first.
> 
> If we can't do union-union inheritance yet, maybe we can work around it this
> way:
> 
> # fields that are always returned by both query-cpus and query-cpus-fast
> { 'struct': 'BothCpuInfoBase',
>   'data': {'cpu': 'int', 'qom_path': 'str', 'thread_id': 'int',
>            '*props': 'CpuInstanceProperties' } }
> 
> # fields that are always returned by query-cpus
> { 'struct': 'CpuInfoBase',
>   'base': 'BothCpuInfoBase',
>   'data': {'current': 'bool', 'halted': 'bool', 'arch': 'CpuInfoArch' } }
> 
> # query-cpus return value
> { 'union': 'CpuInfo',
>   'base': 'CpuInfoBase',
>   'discriminator': 'arch',
>   'data': { 'x86': 'CpuInfoX86',
>             's390': 'CpuInfoS390',
>             'sparc': 'CpuInfoSPARC',
>             'ppc': 'CpuInfoPPC',
>             'mips': 'CpuInfoMIPS',
>             'tricore': 'CpuInfoTricore',
>             'other': 'CpuInfoOther' } }
> 
> # fields that are always returned by query-cpus-fast
> { 'struct': 'FastCpuInfoBase',
>   'base': 'BothCpuInfoBase',
>   'data': { 'arch': 'CpuInfoArch' } }
> 
> # return value of query-cpus-fast
> { 'union': 'FastCpuInfo',
>   'base': 'FastCpuInfoBase',
>   'discriminator': 'arch',
>   'data': { 'x86': 'CpuInfoOther',
>             's390': 'FastCpuInfoS390',
>             'sparc': 'CpuInfoOther',
>             'ppc': 'CpuInfoOther',
>             'mips': 'CpuInfoOther',
>             'tricore': 'CpuInfoOther',
>             'other': 'CpuInfoOther' } }
> 
> # fields returned by both query-cpus and query-cpus-fast on s390
> { 'struct': 'FastCpuInfoS390',
>   'data': { 'fast_field': 'int' } }
> 
> # fields returned by query-cpus on s390
> { 'struct': 'CpuInfoS390',
>   'base': 'FastCpuInfoS390',
>   'data': { 'slow_field': 'int' } }
> 
This is not exactly pretty, but would provide the functionality (and
flexibility) I'd expect.

-- 
Regards,
 Viktor Mihajlovski