| Message ID | 20241205145716.472456-4-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cpu: Drop CPUState::nr_cores | expand |
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? > + > if (ms->smp.modules > 1) { > env->nr_modules = ms->smp.modules; > set_bit(CPU_TOPOLOGY_LEVEL_MODULE, env->avail_cpu_topo); > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 3725dbbc4b3f..15b50c44ae79 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6503,7 +6503,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > > topo_info.dies_per_pkg = env->nr_dies; > topo_info.modules_per_die = env->nr_modules; > - topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules; > + topo_info.cores_per_module = env->nr_cores; > topo_info.threads_per_core = cs->nr_threads; > > cores_per_pkg = topo_info.cores_per_module * topo_info.modules_per_die * > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 5795a497e567..c37a49a1a02b 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -2051,6 +2051,9 @@ typedef struct CPUArchState { > /* Number of modules within one die. */ > unsigned nr_modules; > > + /* Number of cores within one module. */ > + unsigned nr_cores; > + > /* Bitmap of available CPU topology levels for this CPU. */ > DECLARE_BITMAP(avail_cpu_topo, CPU_TOPOLOGY_LEVEL__MAX); > } CPUX86State; > @@ -2393,10 +2396,12 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu, > static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu) > { > CPUState *cs = CPU(cpu); > + CPUX86State *env = &cpu->env; > uint64_t val; > + uint64_t cores_per_package = env->nr_cores * env->nr_modules * env->nr_dies; > > - val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */ > - val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */ > + val = cs->nr_threads * cores_per_package; /* thread count, bits 15..0 */ > + val |= (cores_per_package << 16); /* core count, bits 31..16 */ > > return val; > }
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
On 12/11/2024 12:43 AM, Igor Mammedov wrote: > 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? because env->nr_cores has a different meaning vs. cs->nr_cores. As the below comments of nr_cores in the diff + /* Number of cores within one module. */ + unsigned nr_cores; it means the number of cores within one module. It uses the similar convention as nr_dies and nr_modules in struct CPUArchState. However, CPUState::nr_cores means the number of cores in the package. yes, we can keep the same meaning of nr_cores as "number of cores in the package" when define it struct CPUArchState. However, it leads to inconsistency with nr_dies and nr_modules already there and confuses people. >> + >> if (ms->smp.modules > 1) { >> env->nr_modules = ms->smp.modules; >> set_bit(CPU_TOPOLOGY_LEVEL_MODULE, env->avail_cpu_topo); >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 3725dbbc4b3f..15b50c44ae79 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -6503,7 +6503,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, >> >> topo_info.dies_per_pkg = env->nr_dies; >> topo_info.modules_per_die = env->nr_modules; >> - topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules; >> + topo_info.cores_per_module = env->nr_cores; >> topo_info.threads_per_core = cs->nr_threads; >> >> cores_per_pkg = topo_info.cores_per_module * topo_info.modules_per_die * >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> index 5795a497e567..c37a49a1a02b 100644 >> --- a/target/i386/cpu.h >> +++ b/target/i386/cpu.h >> @@ -2051,6 +2051,9 @@ typedef struct CPUArchState { >> /* Number of modules within one die. */ >> unsigned nr_modules; >> >> + /* Number of cores within one module. */ >> + unsigned nr_cores; >> + >> /* Bitmap of available CPU topology levels for this CPU. */ >> DECLARE_BITMAP(avail_cpu_topo, CPU_TOPOLOGY_LEVEL__MAX); >> } CPUX86State; >> @@ -2393,10 +2396,12 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu, >> static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu) >> { >> CPUState *cs = CPU(cpu); >> + CPUX86State *env = &cpu->env; >> uint64_t val; >> + uint64_t cores_per_package = env->nr_cores * env->nr_modules * env->nr_dies; >> >> - val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */ >> - val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */ >> + val = cs->nr_threads * cores_per_package; /* thread count, bits 15..0 */ >> + val |= (cores_per_package << 16); /* core count, bits 31..16 */ >> >> return val; >> } >
On 12/11/2024 10:50 AM, Zhao Liu wrote: > 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. yes, my plan is to just maintain a "struct X86CPUTopoInfo" in CPUX86State. After we clean up the nr_threads and nr_cores in CPUState usage. > @xiaoyao, do you agree? > > Regards, > Zhao >
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; + if (ms->smp.modules > 1) { env->nr_modules = ms->smp.modules; set_bit(CPU_TOPOLOGY_LEVEL_MODULE, env->avail_cpu_topo); diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 3725dbbc4b3f..15b50c44ae79 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6503,7 +6503,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, topo_info.dies_per_pkg = env->nr_dies; topo_info.modules_per_die = env->nr_modules; - topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules; + topo_info.cores_per_module = env->nr_cores; topo_info.threads_per_core = cs->nr_threads; cores_per_pkg = topo_info.cores_per_module * topo_info.modules_per_die * diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 5795a497e567..c37a49a1a02b 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -2051,6 +2051,9 @@ typedef struct CPUArchState { /* Number of modules within one die. */ unsigned nr_modules; + /* Number of cores within one module. */ + unsigned nr_cores; + /* Bitmap of available CPU topology levels for this CPU. */ DECLARE_BITMAP(avail_cpu_topo, CPU_TOPOLOGY_LEVEL__MAX); } CPUX86State; @@ -2393,10 +2396,12 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu, static inline uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu) { CPUState *cs = CPU(cpu); + CPUX86State *env = &cpu->env; uint64_t val; + uint64_t cores_per_package = env->nr_cores * env->nr_modules * env->nr_dies; - val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */ - val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */ + val = cs->nr_threads * cores_per_package; /* thread count, bits 15..0 */ + val |= (cores_per_package << 16); /* core count, bits 31..16 */ return val; }
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(-)