Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State

[Zhao Liu <zhao1.liu@intel.com>]

On Tue, Dec 10, 2024 at 05:43:38PM +0100, Igor Mammedov wrote:
> Date: Tue, 10 Dec 2024 17:43:38 +0100
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [RFC PATCH 3/4] i386: Track cores_per_module in CPUX86State
> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-redhat-linux-gnu)
> 
> On Thu,  5 Dec 2024 09:57:15 -0500
> Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> 
> > x86 is the only user of CPUState::nr_cores.
> > 
> > Define cores_per_module in CPUX86State, which can serve as the
> > substitute of CPUState::nr_cores. After x86 switches to use
> > CPUX86State::cores_per_module, CPUState::nr_cores will lose the only
> > user and QEMU can drop it.
> > 
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> >  hw/i386/x86-common.c | 2 ++
> >  target/i386/cpu.c    | 2 +-
> >  target/i386/cpu.h    | 9 +++++++--
> >  3 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> > index dc031af66217..f7a20c1da30c 100644
> > --- a/hw/i386/x86-common.c
> > +++ b/hw/i386/x86-common.c
> > @@ -271,6 +271,8 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >  
> >      init_topo_info(&topo_info, x86ms);
> >  
> > +    env->nr_cores = ms->smp.cores;
> this doesn't look like the same as in qemu_init_vcpu(),
> which uses machine_topo_get_cores_per_socket()
> Can you clarify the change?

I think Xiaoyao is correct here. CPUState.nr_cores means number of cores
in socket, and current CPUX86State.nr_cores means number of cores per
module (or parent container) ...though they have same name. (It's better
to mention the such difference in commit message.)

However, I also think that names like nr_cores or nr_* are prone to
errors. Names like cores_per_module are clearer, similar to the naming
in X86CPUTopoInfo. This might be an opportunity to clean up the current
nr_* naming convention.

And further, we can directly cache two additional items in CPUX86State:
threads_per_pkg and cores_per_pkg, as these are the most common
calculations and can help avoid missing any topology levels.

I think both of these changes can be made on top of the current series.

@xiaoyao, do you agree?

Regards,
Zhao