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

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

On 1/18/23 17:08, Daniel P. Berrangé wrote:
> On Wed, Jan 18, 2023 at 04:58:05PM +0100, Pierre Morel wrote:
>>
>>
>> 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.
> 
> That is still inventing a second level data format for an attribute
> that needs to be parsed, and its arguably more complex.

OK, I think I am too focused on my vision of the topology.
I add the new attributes to the query-cpus-fast

> 
>>> 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.
> 
> It shouldn't be that hard to populate a hash table, using the set of
> socket + attributes you want as the hash key.

It is not a problem.

> 
>> I think having both would be interesting.
> 
> IMHO this is undesirable if we can make query-cpus-fast report
> sufficient information, as it gives a maint burden to QEMU and
> is confusing to consumers to QEMU to have multiple commands with
> largely overlapping functionality.

right.

Thanks Daniel.

Regards,
Pierre

> 
> 
> With regards,
> Daniel

-- 
Pierre Morel
IBM Lab Boeblingen