diff mbox series

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

Message ID 20241205145716.472456-4-xiaoyao.li@intel.com (mailing list archive)
State New
Headers show
Series cpu: Drop CPUState::nr_cores | expand

Commit Message

Xiaoyao Li Dec. 5, 2024, 2:57 p.m. UTC
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(-)

Comments

Igor Mammedov Dec. 10, 2024, 4:43 p.m. UTC | #1
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;
>  }
Zhao Liu Dec. 11, 2024, 2:50 a.m. UTC | #2
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
Xiaoyao Li Dec. 12, 2024, 3:30 a.m. UTC | #3
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;
>>   }
>
Xiaoyao Li Dec. 12, 2024, 3:37 a.m. UTC | #4
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 mbox series

Patch

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;
 }