qapi/machine: Fix missing @modules in topology ordering

[PATCH] qapi/machine: Fix missing @modules in topology ordering

Posted by Zhao Liu 1 month ago

The module level is between core and cluster levels. Fix the QAPI
documentation to add the module level in topology ordering.

Reported-by: Markus Armbruster <armbru@redhat.com>
Fixes: 8ec0a4634798 ("hw/core/machine: Support modules in -smp")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 qapi/machine.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 038eab281c78..5e268479e546 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1624,7 +1624,7 @@
 # containers.
 #
 # The ordering from highest/coarsest to lowest/finest is: @drawers,
-# @books, @sockets, @dies, @clusters, @cores, @threads.
+# @books, @sockets, @dies, @clusters, @modules, @cores, @threads.
 #
 # Different architectures support different subsets of topology
 # containers.
-- 
2.34.1

Re: [PATCH] qapi/machine: Fix missing @modules in topology ordering

Posted by Markus Armbruster 1 month ago

Zhao Liu <zhao1.liu@intel.com> writes:

> The module level is between core and cluster levels. Fix the QAPI
> documentation to add the module level in topology ordering.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Fixes: 8ec0a4634798 ("hw/core/machine: Support modules in -smp")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  qapi/machine.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 038eab281c78..5e268479e546 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1624,7 +1624,7 @@
>  # containers.
>  #
>  # The ordering from highest/coarsest to lowest/finest is: @drawers,
> -# @books, @sockets, @dies, @clusters, @cores, @threads.
> +# @books, @sockets, @dies, @clusters, @modules, @cores, @threads.
>  #
>  # Different architectures support different subsets of topology
>  # containers.

Acked-by: Markus Armbruster <armbru@redhat.com>

However, there are more mentions of @drawers etc. in comments and
documentation elsewhere.  Quick grep for "drawers" there appended.
Please double-check for missing mentions of modules.

docs/about/deprecated.rst:configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is
docs/devel/s390-cpu-topology.rst:    -smp 1,drawers=3,books=3,sockets=2,cores=2,maxcpus=36 \
docs/system/s390x/cpu-topology.rst:topology containers: drawers, books and sockets. They define a
docs/system/s390x/cpu-topology.rst:If none of the containers attributes (drawers, books, sockets) are
hw/s390x/cpu-topology.c: * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
include/hw/boards.h: * @drawers_supported - whether drawers are supported by the machine
include/hw/boards.h: * @drawers: the number of drawers on the machine
tests/functional/s390x/test_topology.py:    the cores, sockets, books and drawers and 2 modifiers attributes,
tests/unit/test-smp-parse.c: *  -drawers/books/sockets/cores/threads
tests/unit/test-smp-parse.c: *  -drawers/books/sockets/dies/clusters/modules/cores/threads
tests/unit/test-smp-parse.c:         *   -smp 8,drawers=1,books=1,sockets=2,dies=1,clusters=1,modules=1,\
tests/unit/test-smp-parse.c:        /* config: -smp 2,drawers=2 */
tests/unit/test-smp-parse.c:        /* config: -smp 16,drawers=2,sockets=2,cores=4,threads=2,maxcpus=16 */
tests/unit/test-smp-parse.c:        /* config: -smp 34,drawers=2,sockets=2,cores=4,threads=2,maxcpus=32 */
tests/unit/test-smp-parse.c:         * config: -smp 200,drawers=3,books=5,sockets=2,cores=4,\
tests/unit/test-smp-parse.c:         * config: -smp 242,drawers=3,books=5,sockets=2,cores=4,\
tests/unit/test-smp-parse.c:         * config: -smp 200,drawers=3,books=5,sockets=2,dies=4,\
tests/unit/test-smp-parse.c:         * config: -smp 2881,drawers=3,books=5,sockets=2,dies=4,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=3,books=5,sockets=2,dies=4,\
tests/unit/test-smp-parse.c:         * config: -smp 0,drawers=1,books=1,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:         * Test "drawers=0".
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=0,books=1,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=0,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=0,dies=1,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=0,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
tests/unit/test-smp-parse.c:        /* when drawers parameter is omitted, it will be set as 1 */
tests/unit/test-smp-parse.c:        /* when drawers parameter is specified */
tests/unit/test-smp-parse.c:         * when drawers and books parameters are omitted, they will
tests/unit/test-smp-parse.c:        /* when drawers and books parameters are both specified */
tests/unit/test-smp-parse.c:         * when drawers, books, dies, clusters and modules parameters are
tests/unit/test-smp-parse.c:         * when drawers, books, dies, clusters and modules parameters

Re: [PATCH] qapi/machine: Fix missing @modules in topology ordering

Posted by Zhao Liu 3 weeks, 4 days ago

On Wed, Oct 15, 2025 at 08:21:38AM +0200, Markus Armbruster wrote:
> Date: Wed, 15 Oct 2025 08:21:38 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH] qapi/machine: Fix missing @modules in topology ordering
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > The module level is between core and cluster levels. Fix the QAPI
> > documentation to add the module level in topology ordering.
> >
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Fixes: 8ec0a4634798 ("hw/core/machine: Support modules in -smp")
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >  qapi/machine.json | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 038eab281c78..5e268479e546 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1624,7 +1624,7 @@
> >  # containers.
> >  #
> >  # The ordering from highest/coarsest to lowest/finest is: @drawers,
> > -# @books, @sockets, @dies, @clusters, @cores, @threads.
> > +# @books, @sockets, @dies, @clusters, @modules, @cores, @threads.
> >  #
> >  # Different architectures support different subsets of topology
> >  # containers.
> 
> Acked-by: Markus Armbruster <armbru@redhat.com>

Thanks!

> However, there are more mentions of @drawers etc. in comments and
> documentation elsewhere.  Quick grep for "drawers" there appended.
> Please double-check for missing mentions of modules.
> 
> 
> docs/about/deprecated.rst:configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is

This case should be cleaned up since I rememberred the realted change is
reverted. I will check in details and submit a patch if necessary.

> docs/devel/s390-cpu-topology.rst:    -smp 1,drawers=3,books=3,sockets=2,cores=2,maxcpus=36 \
> docs/system/s390x/cpu-topology.rst:topology containers: drawers, books and sockets. They define a
> docs/system/s390x/cpu-topology.rst:If none of the containers attributes (drawers, books, sockets) are
> hw/s390x/cpu-topology.c: * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).

s390x doesn't support module level, so its doc is fine.

> include/hw/boards.h: * @drawers_supported - whether drawers are supported by the machine
> include/hw/boards.h: * @drawers: the number of drawers on the machine

general machine codes has considerred module level.

> tests/functional/s390x/test_topology.py:    the cores, sockets, books and drawers and 2 modifiers attributes,

s390x doesn't support module level.

> tests/unit/test-smp-parse.c: *  -drawers/books/sockets/cores/threads
> tests/unit/test-smp-parse.c: *  -drawers/books/sockets/dies/clusters/modules/cores/threads
> tests/unit/test-smp-parse.c:         *   -smp 8,drawers=1,books=1,sockets=2,dies=1,clusters=1,modules=1,\
> tests/unit/test-smp-parse.c:        /* config: -smp 2,drawers=2 */
> tests/unit/test-smp-parse.c:        /* config: -smp 16,drawers=2,sockets=2,cores=4,threads=2,maxcpus=16 */
> tests/unit/test-smp-parse.c:        /* config: -smp 34,drawers=2,sockets=2,cores=4,threads=2,maxcpus=32 */
> tests/unit/test-smp-parse.c:         * config: -smp 200,drawers=3,books=5,sockets=2,cores=4,\
> tests/unit/test-smp-parse.c:         * config: -smp 242,drawers=3,books=5,sockets=2,cores=4,\
> tests/unit/test-smp-parse.c:         * config: -smp 200,drawers=3,books=5,sockets=2,dies=4,\
> tests/unit/test-smp-parse.c:         * config: -smp 2881,drawers=3,books=5,sockets=2,dies=4,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=3,books=5,sockets=2,dies=4,\
> tests/unit/test-smp-parse.c:         * config: -smp 0,drawers=1,books=1,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:         * Test "drawers=0".
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=0,books=1,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=0,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=0,dies=1,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=0,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
> tests/unit/test-smp-parse.c:        /* when drawers parameter is omitted, it will be set as 1 */
> tests/unit/test-smp-parse.c:        /* when drawers parameter is specified */
> tests/unit/test-smp-parse.c:         * when drawers and books parameters are omitted, they will
> tests/unit/test-smp-parse.c:        /* when drawers and books parameters are both specified */
> tests/unit/test-smp-parse.c:         * when drawers, books, dies, clusters and modules parameters are
> tests/unit/test-smp-parse.c:         * when drawers, books, dies, clusters and modules parameters

test-smp-parse has coverred module case, so it's fine too. But test-smp-parse
doesn't cover smp-cache. I'll consider to add the new test case.

Thanks very much!

Regards,
Zhao

Re: [PATCH] qapi/machine: Fix missing @modules in topology ordering

Posted by Daniel P. Berrangé 1 month ago

On Mon, Oct 13, 2025 at 03:45:11PM +0800, Zhao Liu wrote:
> The module level is between core and cluster levels. Fix the QAPI
> documentation to add the module level in topology ordering.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Fixes: 8ec0a4634798 ("hw/core/machine: Support modules in -smp")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  qapi/machine.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 038eab281c78..5e268479e546 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1624,7 +1624,7 @@
>  # containers.
>  #
>  # The ordering from highest/coarsest to lowest/finest is: @drawers,
> -# @books, @sockets, @dies, @clusters, @cores, @threads.
> +# @books, @sockets, @dies, @clusters, @modules, @cores, @threads.
>  #
>  # Different architectures support different subsets of topology
>  # containers.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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 :|