Re: [PATCH v2 1/3] hw/smbios: Fix smbios_smp_sockets caculation

[Zhao Liu]

On Wed, Jun 07, 2023 at 04:35:03PM +0200, Igor Mammedov wrote:
> Date: Wed, 7 Jun 2023 16:35:03 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH v2 1/3] hw/smbios: Fix smbios_smp_sockets caculation
> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)

Hi Igor,

> 
> On Thu,  1 Jun 2023 17:29:50 +0800
> Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> 
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Here're 2 mistakes:
> > 1. 003f230e37d7 ("machine: Tweak the order of topology members in struct
> >    CpuTopology") changes the meaning of smp.cores but doesn't fix
> >    original smp.cores uses. And because of the introduction of cluster,
> >    now smp.cores means the number of cores in one cluster. So smp.cores
> >    * smp.threads just means the cpus in a cluster not in a socket.
> 
> > 2. smp.cpus means the number of initial online cpus, not the total
> >    number of cpus. For such topology calculation, smp.max_cpus
> >    should be considered.
> that's probably not relevant to the patch.
> 

For the 2nd point, I mean the original calculation should use max_cpus
other than cpus to calculate sockets:

- smbios_smp_sockets = DIV_ROUND_UP(ms->smp.cpus,
+ smbios_smp_sockets = DIV_ROUND_UP(ms->smp.max_cpus,
                                    ms->smp.cores * ms->smp.threads);


But since we already have smp.sockets, we can use it directly.

> 
> > 
> > Since the number of sockets has already been recorded in smp structure,
> > use smp.sockets directly.
> 
> 
> I'd rephrase commit message to something like this:
> ---
> CPU topology is calculated by ..., and trying to recalculate it here
> with another rules leads to an error, such as 
> 
>  ... example follows ..
> 
> So stop reinventing the another wheel and use topo values that ... has 
> calculated. 

Looks good for me. Thanks!

Regards,
Zhao

> 
> > 
> > Fixes: 003f230e37d7 ("machine: Tweak the order of topology members in 
> > struct CpuTopology")
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >  hw/smbios/smbios.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index d2007e70fb05..d67415d44dd8 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -1088,8 +1088,7 @@ void smbios_get_tables(MachineState *ms,
> >          smbios_build_type_2_table();
> >          smbios_build_type_3_table();
> >  
> > -        smbios_smp_sockets = DIV_ROUND_UP(ms->smp.cpus,
> > -                                          ms->smp.cores * ms->smp.threads);
> > +        smbios_smp_sockets = ms->smp.sockets;
> >          assert(smbios_smp_sockets >= 1);
> >  
> >          for (i = 0; i < smbios_smp_sockets; i++) {
>