diff mbox series

[v26,01/21] qapi: machine.json: change docs regarding CPU topology

Message ID 20231016183925.2384704-2-nsg@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: CPU Topology | expand

Commit Message

Nina Schoetterl-Glausch Oct. 16, 2023, 6:39 p.m. UTC
Clarify roles of different architectures.
Also change things a bit in anticipation of additional members being
added.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 qapi/machine.json | 58 +++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 20 deletions(-)

Comments

Markus Armbruster Oct. 17, 2023, 9:40 a.m. UTC | #1
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:

> Clarify roles of different architectures.
> Also change things a bit in anticipation of additional members being
> added.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>  qapi/machine.json | 58 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index a08b6576ca..058e884fd2 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -71,8 +71,8 @@
>  #
>  # @thread-id: ID of the underlying host thread
>  #
> -# @props: properties describing to which node/socket/core/thread
> -#     virtual CPU belongs to, provided if supported by board
> +# @props: properties of type CpuInstanceProperties associated with a
> +#     virtual CPU, e.g. the socket id

This comes out like

    "props": "CpuInstanceProperties" (optional)
       properties of type CpuInstanceProperties associated with a virtual
       CPU, e.g. the socket id

in generated documentation.

Scratch "of type CpuInstanceProperties, please.

>  #
>  # @target: the QEMU system emulation target, which determines which
>  #     additional fields will be listed (since 3.0)
> @@ -899,28 +899,34 @@
>  # should be passed by management with device_add command when a CPU is
>  # being hotplugged.
>  #
> +# Which members are optional and which mandatory depends on the
> +# architecture and board.
> +#
> +# The ids other than the node-id specify the position of the CPU
> +# within the CPU topology as defined by @SMPConfiguration.

Actually by machine property "smp", which is of type SMPConfiguration.
Not obvious for the reader.  Suggest "as defined by machine property
"smp".

> +#
>  # @node-id: NUMA node ID the CPU belongs to
>  #
> -# @socket-id: socket number within node/board the CPU belongs to
> +# @socket-id: socket number within CPU topology the CPU belongs to
>  #
> -# @die-id: die number within socket the CPU belongs to (since 4.1)
> +# @die-id: die number within the parent container the CPU belongs to
> +#    (since 4.1)
>  #
> -# @cluster-id: cluster number within die the CPU belongs to (since
> -#     7.1)
> +# @cluster-id: cluster number within the parent container the CPU
> +#     belongs to (since 7.1)
>  #
> -# @core-id: core number within cluster the CPU belongs to
> +# @core-id: core number within the parent container the CPU
> +#     belongs to
>  #
> -# @thread-id: thread number within core the CPU belongs to
> +# @thread-id: thread number within the core the CPU  belongs to
>  #
> -# Note: currently there are 6 properties that could be present but
> -#     management should be prepared to pass through other properties
> -#     with device_add command to allow for future interface extension.
> -#     This also requires the filed names to be kept in sync with the
> -#     properties passed to -device/device_add.
> +# Note: management should be prepared to pass through additional
> +#     properties with device_add.
>  #
>  # Since: 2.7
>  ##
>  { 'struct': 'CpuInstanceProperties',
> +  # Keep these in sync with the properties device_add accepts
>    'data': { '*node-id': 'int',
>              '*socket-id': 'int',
>              '*die-id': 'int',
> @@ -1478,21 +1484,33 @@
>  # Schema for CPU topology configuration.  A missing value lets QEMU
>  # figure out a suitable value based on the ones that are provided.
>  #
> +# The members other than @cpus and @maxcpus define topology
> +# containers.

"define a topology of containers"?

"define a hierarchy of containers"?


> +#
> +# The ordering from highest/coarsest to lowest/finest is:
> +# @sockets, @dies, @clusters, @cores, @threads.
> +#
> +# Different architectures support different subsets of topology
> +# containers.
> +#
> +# For examples, s390x does not have clusters and dies, the socket

"For example,"

", and the socket is"

> +# is the parent container of cores.
> +#
>  # @cpus: number of virtual CPUs in the virtual machine
>  #
> +# @maxcpus: maximum number of hotpluggable virtual CPUs in the virtual
> +#     machine
> +#
>  # @sockets: number of sockets in the CPU topology
>  #
> -# @dies: number of dies per socket in the CPU topology
> +# @dies: number of dies per parent container
>  #
> -# @clusters: number of clusters per die in the CPU topology (since
> +# @clusters: number of clusters per parent container (since
>  #     7.0)

This now fits in the previous line:

   # @clusters: number of clusters per parent container (since 7.0)

>  #
> -# @cores: number of cores per cluster in the CPU topology
> +# @cores: number of cores per parent container
>  #
> -# @threads: number of threads per core in the CPU topology
> -#
> -# @maxcpus: maximum number of hotpluggable virtual CPUs in the virtual
> -#     machine
> +# @threads: number of threads per core

Much clearer than before.

>  #
>  # Since: 6.1
>  ##

Address my nitpicks one way or the other, and you may add
Acked-by: Markus Armbruster <armbru@redhat.com>
Thomas Huth Oct. 17, 2023, 11:10 a.m. UTC | #2
On 17/10/2023 11.40, Markus Armbruster wrote:
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
> 
>> Clarify roles of different architectures.
>> Also change things a bit in anticipation of additional members being
>> added.
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> ---
>>   qapi/machine.json | 58 +++++++++++++++++++++++++++++++----------------
>>   1 file changed, 38 insertions(+), 20 deletions(-)
...
> 
> Address my nitpicks one way or the other, and you may add
> Acked-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks Markus!

Nina, if you agree, I can fix the nitpicks while picking the patches up, so 
you don't have to respin again?

  Thomas
Nina Schoetterl-Glausch Oct. 19, 2023, 9:57 a.m. UTC | #3
On Tue, 2023-10-17 at 13:10 +0200, Thomas Huth wrote:
> On 17/10/2023 11.40, Markus Armbruster wrote:
> > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
> > 
> > > Clarify roles of different architectures.
> > > Also change things a bit in anticipation of additional members being
> > > added.
> > > 
> > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > > ---
> > >   qapi/machine.json | 58 +++++++++++++++++++++++++++++++----------------
> > >   1 file changed, 38 insertions(+), 20 deletions(-)
> ...
> > 
> > Address my nitpicks one way or the other, and you may add
> > Acked-by: Markus Armbruster <armbru@redhat.com>
> > 
> 
> Thanks Markus!
> 
> Nina, if you agree, I can fix the nitpicks while picking the patches up, so 
> you don't have to respin again?

Thanks for going ahead with it! I was out sick the last couple of days.

> 
>   Thomas
>
diff mbox series

Patch

diff --git a/qapi/machine.json b/qapi/machine.json
index a08b6576ca..058e884fd2 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -71,8 +71,8 @@ 
 #
 # @thread-id: ID of the underlying host thread
 #
-# @props: properties describing to which node/socket/core/thread
-#     virtual CPU belongs to, provided if supported by board
+# @props: properties of type CpuInstanceProperties associated with a
+#     virtual CPU, e.g. the socket id
 #
 # @target: the QEMU system emulation target, which determines which
 #     additional fields will be listed (since 3.0)
@@ -899,28 +899,34 @@ 
 # should be passed by management with device_add command when a CPU is
 # being hotplugged.
 #
+# Which members are optional and which mandatory depends on the
+# architecture and board.
+#
+# The ids other than the node-id specify the position of the CPU
+# within the CPU topology as defined by @SMPConfiguration.
+#
 # @node-id: NUMA node ID the CPU belongs to
 #
-# @socket-id: socket number within node/board the CPU belongs to
+# @socket-id: socket number within CPU topology the CPU belongs to
 #
-# @die-id: die number within socket the CPU belongs to (since 4.1)
+# @die-id: die number within the parent container the CPU belongs to
+#    (since 4.1)
 #
-# @cluster-id: cluster number within die the CPU belongs to (since
-#     7.1)
+# @cluster-id: cluster number within the parent container the CPU
+#     belongs to (since 7.1)
 #
-# @core-id: core number within cluster the CPU belongs to
+# @core-id: core number within the parent container the CPU
+#     belongs to
 #
-# @thread-id: thread number within core the CPU belongs to
+# @thread-id: thread number within the core the CPU  belongs to
 #
-# Note: currently there are 6 properties that could be present but
-#     management should be prepared to pass through other properties
-#     with device_add command to allow for future interface extension.
-#     This also requires the filed names to be kept in sync with the
-#     properties passed to -device/device_add.
+# Note: management should be prepared to pass through additional
+#     properties with device_add.
 #
 # Since: 2.7
 ##
 { 'struct': 'CpuInstanceProperties',
+  # Keep these in sync with the properties device_add accepts
   'data': { '*node-id': 'int',
             '*socket-id': 'int',
             '*die-id': 'int',
@@ -1478,21 +1484,33 @@ 
 # Schema for CPU topology configuration.  A missing value lets QEMU
 # figure out a suitable value based on the ones that are provided.
 #
+# The members other than @cpus and @maxcpus define topology
+# containers.
+#
+# The ordering from highest/coarsest to lowest/finest is:
+# @sockets, @dies, @clusters, @cores, @threads.
+#
+# Different architectures support different subsets of topology
+# containers.
+#
+# For examples, s390x does not have clusters and dies, the socket
+# is the parent container of cores.
+#
 # @cpus: number of virtual CPUs in the virtual machine
 #
+# @maxcpus: maximum number of hotpluggable virtual CPUs in the virtual
+#     machine
+#
 # @sockets: number of sockets in the CPU topology
 #
-# @dies: number of dies per socket in the CPU topology
+# @dies: number of dies per parent container
 #
-# @clusters: number of clusters per die in the CPU topology (since
+# @clusters: number of clusters per parent container (since
 #     7.0)
 #
-# @cores: number of cores per cluster in the CPU topology
+# @cores: number of cores per parent container
 #
-# @threads: number of threads per core in the CPU topology
-#
-# @maxcpus: maximum number of hotpluggable virtual CPUs in the virtual
-#     machine
+# @threads: number of threads per core
 #
 # Since: 6.1
 ##