[03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

Commit Message

Zhao Liu March 6, 2024, 9:53 a.m. UTC

From: Zhao Liu <[e-mail]>

SMPConfiguration initializes its int64_t members as 0 by default.

Therefore, in machine_parse_smp_config(), initialize local topology
variables with SMPConfiguration's members directly.

Suggested-by: Prasad Pandit <[e-mail]>
Signed-off-by: Zhao Liu <[e-mail]>
---
 hw/core/machine-smp.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Thomas Huth March 8, 2024, 1:20 p.m. UTC | #1

On 06/03/2024 10.53, Zhao Liu wrote:
> From: Zhao Liu <[e-mail]>
> 
> SMPConfiguration initializes its int64_t members as 0 by default.

Can we always rely on that? ... or is this just by luck due to the current 
implementation? In the latter case, I'd maybe rather drop this patch again.

  Thomas


> Therefore, in machine_parse_smp_config(), initialize local topology
> variables with SMPConfiguration's members directly.
> 
> Suggested-by: Prasad Pandit <[e-mail]>
> Signed-off-by: Zhao Liu <[e-mail]>
> ---
>   hw/core/machine-smp.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 50a5a40dbc3d..3d9799aef039 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -82,15 +82,15 @@ void machine_parse_smp_config(MachineState *ms,
>                                 const SMPConfiguration *config, Error **errp)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(ms);
> -    unsigned cpus    = config->has_cpus ? config->cpus : 0;
> -    unsigned drawers = config->has_drawers ? config->drawers : 0;
> -    unsigned books   = config->has_books ? config->books : 0;
> -    unsigned sockets = config->has_sockets ? config->sockets : 0;
> -    unsigned dies    = config->has_dies ? config->dies : 0;
> -    unsigned clusters = config->has_clusters ? config->clusters : 0;
> -    unsigned cores   = config->has_cores ? config->cores : 0;
> -    unsigned threads = config->has_threads ? config->threads : 0;
> -    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> +    unsigned cpus     = config->cpus;
> +    unsigned drawers  = config->drawers;
> +    unsigned books    = config->books;
> +    unsigned sockets  = config->sockets;
> +    unsigned dies     = config->dies;
> +    unsigned clusters = config->clusters;
> +    unsigned cores    = config->cores;
> +    unsigned threads  = config->threads;
> +    unsigned maxcpus  = config->maxcpus;
>   
>       /*
>        * Specified CPU topology parameters must be greater than zero,

Zhao Liu March 8, 2024, 3:33 p.m. UTC | #2

Hi Thomas,

On Fri, Mar 08, 2024 at 02:20:45PM +0100, Thomas Huth wrote:
> Date: Fri, 8 Mar 2024 14:20:45 +0100
> From: Thomas Huth <[e-mail]>
> Subject: Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables'
>  initialization in machine_parse_smp_config()
> 
> On 06/03/2024 10.53, Zhao Liu wrote:
> > From: Zhao Liu <[e-mail]>
> > 
> > SMPConfiguration initializes its int64_t members as 0 by default.
> 
> Can we always rely on that? ... or is this just by luck due to the current
> implementation? In the latter case, I'd maybe rather drop this patch again.
>

Thanks for the correction, I revisited and referenced more similar use
cases, and indeed, only if the flag "has_*" is true, its corresponding
field should be considered reliable.

Keeping explicit checking on has_* and explicit initialization of these
topology variables makes the code more readable.

This patch is over-optimized and I would drop it.

Regards,
Zhao

Prasad Pandit March 10, 2024, 11:55 a.m. UTC | #3

Hi,

On Fri, 8 Mar 2024 at 20:50, Zhao Liu <[e-mail]> wrote:
> On Fri, Mar 08, 2024 at 02:20:45PM +0100, Thomas Huth wrote:
> > Can we always rely on that? ... or is this just by luck due to the current
> > implementation? In the latter case, I'd maybe rather drop this patch again.
>
> Thanks for the correction, I revisited and referenced more similar use
> cases, and indeed, only if the flag "has_*" is true, its corresponding
> field should be considered reliable.

* Is this because 'SMPConfiguration config'  fields are not always
initialized with default values? Is that a bug? Having
'SMPConfiguration' fields initialised to known default values could
help to unify/simplify code which uses those fields.

> Keeping explicit checking on has_* and explicit initialization of these
> topology variables makes the code more readable.
>
> This patch is over-optimized and I would drop it.

* Could we then simplify it in the following if <expression>
===
      if ((config->has_cpus && config->cpus == 0) ||
          ... ||
          (config->has_maxcpus && config->maxcpus == 0))

could be

      if (!cpus || !drawers || ... || !maxcpus) { ... }
===

Thank you.
---
  - Prasad

Zhao Liu March 11, 2024, 5:20 a.m. UTC | #4

Hi Prasad (and also welcome folks correct me),

> On Fri, 8 Mar 2024 at 20:50, Zhao Liu <[e-mail]> wrote:
> > On Fri, Mar 08, 2024 at 02:20:45PM +0100, Thomas Huth wrote:
> > > Can we always rely on that? ... or is this just by luck due to the current
> > > implementation? In the latter case, I'd maybe rather drop this patch again.
> >
> > Thanks for the correction, I revisited and referenced more similar use
> > cases, and indeed, only if the flag "has_*" is true, its corresponding
> > field should be considered reliable.
> 
> * Is this because 'SMPConfiguration config'  fields are not always
> initialized with default values?

SMPConfiguration is created and set in machine_set_smp().

Firstly, it is created by g_autoptr(), and then it is initialized by
visit_type_SMPConfiguration().

The visit_type_SMPConfiguration() is generated by
gen_visit_object_members() in scripts/qapi/visit.py.

The g_autoptr() requires user to initialize:

  You must initialise the variable in some way — either by use of an
  initialiser or by ensuring that it is assigned to unconditionally
  before it goes out of scope.

This means if user doesn't initialize some field, the the value should
be considerred as unreliable. And I guess for int, the default
initialization value is the same as if we had declared a regular integer
variable. But anyway, fields that are not explicitly initialized should
not be accessed.

IIUC, in visit_type_SMPConfiguration() (let's have a look at
gen_visit_object_members()), there's no explicit initialization of
SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in
gen_visit_object_members()). For int type, has_* means that the field is
set.

Therefore, we shouldn't rely on the uninitialized fields and should
check the has_*.

> Is that a bug?

It's not a bug, and it could be a simplification of the automatic code
generation.

> Having 'SMPConfiguration' fields initialised to known default values could
> help to unify/simplify code which uses those fields.

I realized that keeping the check for has_* here would help improve code
readability; after all, it's more of a pain to go back and check
scripts.

> > Keeping explicit checking on has_* and explicit initialization of these
> > topology variables makes the code more readable.
> >
> > This patch is over-optimized and I would drop it.
> 
> * Could we then simplify it in the following if <expression>
> ===
>       if ((config->has_cpus && config->cpus == 0) ||
>           ... ||
>           (config->has_maxcpus && config->maxcpus == 0))
> 
> could be
> 
>       if (!cpus || !drawers || ... || !maxcpus) { ... }
> ===
>

This doesn't work since the above check is used to identify if user sets
parameters as 0 explicitly, which needs to check both has_* and the
specific field value.

(Communicating with you also helped me to understand the QAPI related
parts.)

Thanks,
Zhao

Prasad Pandit March 13, 2024, 10:52 a.m. UTC | #5

Hello Zhao,

> (Communicating with you also helped me to understand the QAPI related parts.)

* I'm also visiting QAPI code parts for the first time. Thanks to you. :)

On Mon, 11 Mar 2024 at 10:36, Zhao Liu <[e-mail]> wrote:
> SMPConfiguration is created and set in machine_set_smp().
> Firstly, it is created by g_autoptr(), and then it is initialized by
> visit_type_SMPConfiguration().
>
> The visit_type_SMPConfiguration() is generated by
> gen_visit_object_members() in scripts/qapi/visit.py.
>
> IIUC, in visit_type_SMPConfiguration() (let's have a look at
> gen_visit_object_members()), there's no explicit initialization of
> SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in
> gen_visit_object_members()). For int type, has_* means that the field is
> set.
>

* Thank you for the above details, appreciate it. I added few
fprintf() calls to visit_type_SMPConfiguration() to see what user
values it receives
===
$ ./qemu-system-x86_64 -smp cores=2,maxcpus=2,cpus=1,sockets=2
visit_type_SMPConfiguration: name: smp
        has_cpus: 1:1
 has_drawvers: 0:0
      has_books: 0:0
  has_sockets: 1:2
        has_dies: 0:0
 has_clusters: 0:0
     has_cores: 1:2
  has_threads: 0:0
has_maxcpus: 1:2
qemu-system-x86_64: Invalid CPU topology: product of the hierarchy
must match maxcpus: sockets (2) * dies (1) * cores (2) * threads (0)
!= maxcpus (2)
===
* As seen above, undefined -smp fields (both has_xxxx and xxxx) are
set to zero(0).

===
main
 qemu_init
  qemu_apply_machine_options
   object_set_properties_from_keyval
    object_set_properties_from_qdict
     object_property_set
      machine_set_smp
       visit_type_SMPConfiguration
        visit_start_struct
(gdb) p v->start_struct
$4 = ... 0x5555570248e4 <qobject_input_start_struct>
(gdb)
(gdb)
 qobject_input_start_struct
   if (obj) {
        *obj = g_malloc0(size);
    }
===
* Stepping through function calls in gdb(1) revealed above call
sequence which leads to  'SMPConfiguration **'  objects allocation by
g_malloc0.

> This means if user doesn't initialize some field, the the value should
> be considerred as unreliable. And I guess for int, the default
> initialization value is the same as if we had declared a regular integer
> variable. But anyway, fields that are not explicitly initialized should
> not be accessed.

* g_malloc0() allocating 'SMPConfiguration **' object above assures us
that undefined -smp fields shall always be zero(0).

* 'obj->has_xxxx' field is set only if the user has supplied the
variable value, otherwise it remains set to zero(0).
   visit_type_SMPConfiguration_members
     ->visit_optional
       ->v->optional
        -> qobject_input_optional

* Overall, I think there is scope to simplify the
'machine_parse_smp_config()' function by using SMPConfiguration
field(s) ones.

Thank you.
---
  - Prasad

Zhao Liu March 18, 2024, 8:06 a.m. UTC | #6

On Wed, Mar 13, 2024 at 04:22:30PM +0530, Prasad Pandit wrote:
> Date: Wed, 13 Mar 2024 16:22:30 +0530
> From: Prasad Pandit <[e-mail]>
> Subject: Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables'
>  initialization in machine_parse_smp_config()
> 
> Hello Zhao,
> 
> > (Communicating with you also helped me to understand the QAPI related parts.)
> 
> * I'm also visiting QAPI code parts for the first time. Thanks to you. :)
> 
> On Mon, 11 Mar 2024 at 10:36, Zhao Liu <[e-mail]> wrote:
> > SMPConfiguration is created and set in machine_set_smp().
> > Firstly, it is created by g_autoptr(), and then it is initialized by
> > visit_type_SMPConfiguration().
> >
> > The visit_type_SMPConfiguration() is generated by
> > gen_visit_object_members() in scripts/qapi/visit.py.
> >
> > IIUC, in visit_type_SMPConfiguration() (let's have a look at
> > gen_visit_object_members()), there's no explicit initialization of
> > SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in
> > gen_visit_object_members()). For int type, has_* means that the field is
> > set.
> >
> 
> * Thank you for the above details, appreciate it. I added few
> fprintf() calls to visit_type_SMPConfiguration() to see what user
> values it receives
> ===
> $ ./qemu-system-x86_64 -smp cores=2,maxcpus=2,cpus=1,sockets=2
> visit_type_SMPConfiguration: name: smp
>         has_cpus: 1:1
>  has_drawvers: 0:0
>       has_books: 0:0
>   has_sockets: 1:2
>         has_dies: 0:0
>  has_clusters: 0:0
>      has_cores: 1:2
>   has_threads: 0:0
> has_maxcpus: 1:2
> qemu-system-x86_64: Invalid CPU topology: product of the hierarchy
> must match maxcpus: sockets (2) * dies (1) * cores (2) * threads (0)
> != maxcpus (2)
> ===
> * As seen above, undefined -smp fields (both has_xxxx and xxxx) are
> set to zero(0).
> 
> ===
> main
>  qemu_init
>   qemu_apply_machine_options
>    object_set_properties_from_keyval
>     object_set_properties_from_qdict
>      object_property_set
>       machine_set_smp
>        visit_type_SMPConfiguration
>         visit_start_struct
> (gdb) p v->start_struct
> $4 = ... 0x5555570248e4 <qobject_input_start_struct>
> (gdb)
> (gdb)
>  qobject_input_start_struct
>    if (obj) {
>         *obj = g_malloc0(size);
>     }
> ===
> * Stepping through function calls in gdb(1) revealed above call
> sequence which leads to  'SMPConfiguration **'  objects allocation by
> g_malloc0.

Thanks! I misunderstood, it turns out that the initialization is done here.

> 
> > This means if user doesn't initialize some field, the the value should
> > be considerred as unreliable. And I guess for int, the default
> > initialization value is the same as if we had declared a regular integer
> > variable. But anyway, fields that are not explicitly initialized should
> > not be accessed.
> 
> * g_malloc0() allocating 'SMPConfiguration **' object above assures us
> that undefined -smp fields shall always be zero(0).
> 
> * 'obj->has_xxxx' field is set only if the user has supplied the
> variable value, otherwise it remains set to zero(0).
>    visit_type_SMPConfiguration_members
>      ->visit_optional
>        ->v->optional
>         -> qobject_input_optional

Yes, you're right!

> 
> * Overall, I think there is scope to simplify the
> 'machine_parse_smp_config()' function by using SMPConfiguration
> field(s) ones.

Indeed, as you say, these items are initialized to 0 by default.

I think, however, that the initialization is so far away from where the
smp is currently parsed that one can't easily confirm it (thanks for
your confirmation!).

From a code readability view, the fact that we're explicitly
initializing to 0 again here brings little overhead, but makes the code
more readable as well as easier to maintain. I think the small redundancy
here is worth it.

Also, in other use cases people always relies on fields marked by has_*,
and there is no (or less?) precedent for direct access to places where
has_* is not set. I understand that this is also a habit, i.e., fields
with a has_* of False by default are unreliable and avoid going directly
to them.

Regards,
Zhao

Prasad Pandit March 18, 2024, 11:18 a.m. UTC | #7

Hello,

On Mon, 18 Mar 2024 at 13:23, Zhao Liu <[e-mail]> wrote:
> Indeed, as you say, these items are initialized to 0 by default.
>
> I think, however, that the initialization is so far away from where the
> smp is currently parsed that one can't easily confirm it (thanks for
> your confirmation!).
>
> From a code readability view, the fact that we're explicitly
> initializing to 0 again here brings little overhead, but makes the code
> more readable as well as easier to maintain. I think the small redundancy
> here is worth it.
>
> Also, in other use cases people always relies on fields marked by has_*,
> and there is no (or less?) precedent for direct access to places where
> has_* is not set. I understand that this is also a habit, i.e., fields
> with a has_* of False by default are unreliable and avoid going directly
> to them.

* Ummn...okay. (I'm not fully convinced, but that's fine, I'm okay to
go with you on this.)

Thank you.
---
  - Prasad

Patch

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 50a5a40dbc3d..3d9799aef039 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -82,15 +82,15 @@  void machine_parse_smp_config(MachineState *ms,
                               const SMPConfiguration *config, Error **errp)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
-    unsigned cpus    = config->has_cpus ? config->cpus : 0;
-    unsigned drawers = config->has_drawers ? config->drawers : 0;
-    unsigned books   = config->has_books ? config->books : 0;
-    unsigned sockets = config->has_sockets ? config->sockets : 0;
-    unsigned dies    = config->has_dies ? config->dies : 0;
-    unsigned clusters = config->has_clusters ? config->clusters : 0;
-    unsigned cores   = config->has_cores ? config->cores : 0;
-    unsigned threads = config->has_threads ? config->threads : 0;
-    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
+    unsigned cpus     = config->cpus;
+    unsigned drawers  = config->drawers;
+    unsigned books    = config->books;
+    unsigned sockets  = config->sockets;
+    unsigned dies     = config->dies;
+    unsigned clusters = config->clusters;
+    unsigned cores    = config->cores;
+    unsigned threads  = config->threads;
+    unsigned maxcpus  = config->maxcpus;
 
     /*
      * Specified CPU topology parameters must be greater than zero,