| Message ID | 20230601092952.1114727-2-zhao1.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/smbios: Cleanup topology related variables | expand |
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. > > 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. > > 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++) {
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++) { >
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++) {