Re: [PATCH v14 09/11] qapi/s390/cpu topology: monitor query topology information

[Pierre Morel <pmorel@linux.ibm.com>]

On 1/12/23 13:10, Daniel P. Berrangé wrote:
> On Thu, Jan 05, 2023 at 03:53:11PM +0100, Pierre Morel wrote:
>> Reporting the current topology informations to the admin through
>> the QEMU monitor.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   qapi/machine-target.json | 66 ++++++++++++++++++++++++++++++++++
>>   include/monitor/hmp.h    |  1 +
>>   hw/s390x/cpu-topology.c  | 76 ++++++++++++++++++++++++++++++++++++++++
>>   hmp-commands-info.hx     | 16 +++++++++
>>   4 files changed, 159 insertions(+)
>>
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index 75b0aa254d..927618a78f 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -371,3 +371,69 @@
>>     },
>>     'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
>>   }
>> +
>> +##
>> +# @S390CpuTopology:
>> +#
>> +# CPU Topology information
>> +#
>> +# @drawer: the destination drawer where to move the vCPU
>> +#
>> +# @book: the destination book where to move the vCPU
>> +#
>> +# @socket: the destination socket where to move the vCPU
>> +#
>> +# @polarity: optional polarity, default is last polarity set by the guest
>> +#
>> +# @dedicated: optional, if the vCPU is dedicated to a real CPU
>> +#
>> +# @origin: offset of the first bit of the core mask
>> +#
>> +# @mask: mask of the cores sharing the same topology
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'struct': 'S390CpuTopology',
>> +  'data': {
>> +      'drawer': 'int',
>> +      'book': 'int',
>> +      'socket': 'int',
>> +      'polarity': 'int',
>> +      'dedicated': 'bool',
>> +      'origin': 'int',
>> +      'mask': 'str'
>> +  },
>> +  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
>> +}
>> +
>> +##
>> +# @query-topology:
>> +#
>> +# Return information about CPU Topology
>> +#
>> +# Returns a @CpuTopology instance describing the CPU Toplogy
>> +# being currently used by QEMU.
>> +#
>> +# Since: 8.0
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "cpu-topology" }
>> +# <- {"return": [
>> +#     {
>> +#         "drawer": 0,
>> +#         "book": 0,
>> +#         "socket": 0,
>> +#         "polarity": 0,
>> +#         "dedicated": true,
>> +#         "origin": 0,
>> +#         "mask": 0xc000000000000000,
>> +#     },
>> +#    ]
>> +#   }
>> +#
>> +##
>> +{ 'command': 'query-topology',
>> +  'returns': ['S390CpuTopology'],
>> +  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
>> +}
> 
> IIUC, you're using @mask as a way to compress the array returned
> from query-topology, so that it doesn't have any repeated elements
> with the same data. I guess I can understand that desire when the
> core count can get very large, this can have a large saving.
> 
> The downside of using @mask, is that now you require the caller
> to parse the string to turn it into a bitmask and expand the
> data. Generally this is considered a bit of an anti-pattern in
> QAPI design - we don't want callers to have to further parse
> the data to extract information, we want to directly consumable
> from the parsed JSON doc.

Not exactly, the mask is computed by the firmware to provide it to the 
guest and is already available when querying the topology.
But I understand that for the QAPI user the mask is not the right 
solution, standard coma separated values like (1,3,5,7-11) would be much 
easier to read.

> 
> We already have 'query-cpus-fast' wich returns one entry for
> each CPU. In fact why do we need to add query-topology at all.
> Can't we just add book-id / drawer-id / polarity / dedicated
> to the query-cpus-fast result ?

Yes we can, I think we should, however when there are a lot of CPU it 
will be complicated to find the CPU sharing the same socket and the same 
attributes.
I think having both would be interesting.

What do you think?

regards,
Pierre

> 
> With regards,
> Daniel

-- 
Pierre Morel
IBM Lab Boeblingen