Re: [PATCH v25 02/21] CPU topology: extend with s390 specifics

[Markus Armbruster]

Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:

> On Thu, 2023-10-12 at 13:02 +0200, Markus Armbruster wrote:
>> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
>> 
>> > From: Pierre Morel <pmorel@linux.ibm.com>
>> > 
>> > S390 adds two new SMP levels, drawers and books to the CPU
>> > topology.
>> > S390 CPUs have specific topology features like dedication and
>> > entitlement. These indicate to the guest information on host
>> > vCPU scheduling and help the guest make better scheduling decisions.
>> > 
>> > Let us provide the SMP properties with books and drawers levels
>> > and S390 CPU with dedication and entitlement,
>> 
>> This is vague.  Peeking at the patch, I can see it adds properties
>> "socket-id", "book-id", "drawer-id", "dedicated", and "entitlement" to
>> "s390x-cpu" objects.  Suggest to spell that out here.
>> 
>> > Add machine-common.json so we can later include it in
>> > machine-target.json also.
>> > 
>> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> > Reviewed-by: Thomas Huth <thuth@redhat.com>
>> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> > ---
>> >  MAINTAINERS                         |  1 +
>> >  qapi/machine-common.json            | 21 +++++++++++++
>> >  qapi/machine.json                   | 17 +++++++++-
>> >  qapi/qapi-schema.json               |  1 +
>> >  include/hw/boards.h                 | 10 +++++-
>> >  include/hw/qdev-properties-system.h |  4 +++
>> >  target/s390x/cpu.h                  |  6 ++++
>> >  hw/core/machine-smp.c               | 48 ++++++++++++++++++++++++-----
>> >  hw/core/machine.c                   |  4 +++
>> >  hw/core/qdev-properties-system.c    | 13 ++++++++
>> >  hw/s390x/s390-virtio-ccw.c          |  4 +++
>> >  softmmu/vl.c                        |  6 ++++
>> >  target/s390x/cpu.c                  |  7 +++++
>> >  qapi/meson.build                    |  1 +
>> >  qemu-options.hx                     |  7 +++--
>> >  15 files changed, 139 insertions(+), 11 deletions(-)
>> >  create mode 100644 qapi/machine-common.json
>> > 
>> > diff --git a/MAINTAINERS b/MAINTAINERS
>> > index 81625f036b..3f6888aa86 100644
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -1775,6 +1775,7 @@ F: hw/core/null-machine.c
>> >  F: hw/core/numa.c
>> >  F: hw/cpu/cluster.c
>> >  F: qapi/machine.json
>> > +F: qapi/machine-common.json
>> >  F: qapi/machine-target.json
>> >  F: include/hw/boards.h
>> >  F: include/hw/core/cpu.h
>> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
>> > new file mode 100644
>> > index 0000000000..fa6bd71d12
>
> [...] 
>
>> >  ##
>> >  # @SysEmuTarget:
>> > @@ -904,7 +905,13 @@
>> >  #
>> >  # @node-id: NUMA node ID the CPU belongs to
>> >  #
>> > -# @socket-id: socket number within node/board the CPU belongs to
>> > +# @drawer-id: drawer number within node/board the CPU belongs to
>> > +#     (since 8.2)
>> > +#
>> > +# @book-id: book number within drawer/node/board the CPU belongs to
>> > +#     (since 8.2)
>> > +#
>> > +# @socket-id: socket number within book/node/board the CPU belongs to
>> >  #
>> >  # @die-id: die number within socket the CPU belongs to (since 4.1)
>> >  #
>>    # @cluster-id: cluster number within die the CPU belongs to (since
>>    #     7.1)
>>    #
>>    # @core-id: core number within cluster the CPU belongs to
>>    #
>>    # @thread-id: thread number within core the CPU belongs to
>> 
>> So...
>> 
>> * A thread can only be within a core
>> 
>> * A core can only be within a cluster
>> 
>> * A cluster can only be within a die
>> 
>> * A die can only be within a socket
>> 
>> * A socket can be within a book, node, or board
>> 
>> * A book can be within a drawer, node, or board
>> 
>> * A drawer can be within a node, or board
>> 
>> * A node is a NUMA node
>> 
>> * A board is what exactly?  can we have more than one?  is node always
>>   within a/the board?
>
> Yeah, the description is confusing.
>> 
>> Asked differently: what are the possible hierarchies of things?
>
> The way I understand things is:
> * Different architectures have different hierarchies, say
>   1. (thread, core, cluster, die, socket)
>   2. (thread, core, socket, book, drawer)
>
> We define a qemu artificial ordered super set
> (thread, core, cluster, die, socket, book, drawer)
> where architectures can choose a subset of, specifying that they
> support a certain level or not.
>
> Now if for example x86 wanted to support a book level between
> thread and core, we'd need to change a bunch of code and make
> things more complicated.
>
> The NUMA node-id maps a hierarchy tuple to a node, I don't think
> it's part of hierarchy itself.
>
> Now the question is how to document this.

Right.

> On s390x there is no cluster, so what does
>
> @core-id: core number within cluster the CPU belongs to
>
> mean?
>
> We could say, that within the qemu super set there is a virtual
> cluster of which there is one per die (and one die per socket).
>
> Or we rewrite the documentation to say
>
> @x-id: x number within the upper hierarchy container
>
> to account for the fact that the upper container is different
> on different architectures.

Perhaps a structure like this could work.

1. Define the most general hierarchy.

2. Explain the actual hierarchy is a subset.

3. Explain the members as "number within container", as you proposed.

Only works if a superset of all the actual hierarchies exists, and is
sufficiently easy explain.

>> > @@ -923,6 +930,8 @@
>> >  { 'struct': 'CpuInstanceProperties',
>> >    # Keep these in sync with the properties device_add accepts
>> >    'data': { '*node-id': 'int',
>> > +            '*drawer-id': 'int',
>> > +            '*book-id': 'int',
>> >              '*socket-id': 'int',
>> >              '*die-id': 'int',
>> >              '*cluster-id': 'int',
>> > @@ -1481,6 +1490,10 @@
>> >  #
>> >  # @cpus: number of virtual CPUs in the virtual machine
>> >  #
>> > +# @drawers: number of drawers in the CPU topology (since 8.2)
>> > +#
>> > +# @books: number of books in the CPU topology (since 8.2)
>> > +#
>> >  # @sockets: number of sockets in the CPU topology
>> 
>> Total numer of sockets?  Or number of sockets per whatever thing
>> contains sockets?
>
> The latter. I'll change this
>> 
>> Same question for @books, @drawers, and @cpus.
>
> Same for the first two, total for @cpus.
>> 
>> The documentation is less than clear before your patch; your patch
>> merely makes me look at it.  We may decide that addressing the lack of
>> clarity is not your patch's job, and leave it for later.
>
> Yeah, same problem here around different architectures using different sub 
> sets.
>> 
>> >  #
>> >  # @dies: number of dies per socket in the CPU topology
>> > @@ -1499,6 +1512,8 @@
>> >  ##
>> >  { 'struct': 'SMPConfiguration', 'data': {
>> >       '*cpus': 'int',
>> > +     '*drawers': 'int',
>> > +     '*books': 'int',
>> >       '*sockets': 'int',
>> >       '*dies': 'int',
>> >       '*clusters': 'int',
>
> [...]

Thanks!