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@xxxxxxxxxxxxx>
> > > ---
> > > 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.
> > 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.
> 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.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
