Re: [RFC PATCH 0/4] cpu: Drop CPUState::nr_cores

[Xiaoyao Li]

On 12/31/2024 12:11 AM, Igor Mammedov wrote:
> On Thu,  5 Dec 2024 09:57:12 -0500
> Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> > The series is motivated by auditing the usage of CPUState::nr_cores and
> > CPUState::nr_threads, which is motivated by [1].
> >
> > The initial goal is to initialize nr_threads and nr_cores earlier for
> > x86, which leads to patches [2] and [3]. Patch [2] touches all the ARCHes
> > and patch [3] looks hacky. At last Igor suggested to audit nr_threads and
> > nr_cores, and only set them in the pre_plug() callback for the ARCHes that
> > really need them[1].
> >
> > By audting nr_threads and nr_cores, I found nr_cores is only used by
> > x86. So we can introduce a x86 specific one and initialize in
> > x86_cpu_pre_plug(), then drop nr_cores totally.
> >
> > However for nr_threads, it's used by MIPS and PPC as well[4]. There are
> > two options:
> > 1. maintain separate substitute in X86/MIPS/PPS, so we can drop
> > CPUState::nr_threads like for CPUState::nr_cores.
> >
> > 2. keep CPUState::nr_threads and find place (or introduce pre_plug()) to
> > initialize them earlier for MIPS/PPC.
> >
> > I would like to seek for opinions for which one is prefered.
> >
> > This series implments the drop for CPUState::nr_cores. Though it doesn't
> > help on the initial goal without the solution for nr_threads, I think it
> > is still a good cleanup?
> >
> > BTW, by initializing nr_threads and nr_cores earlier than
> > qemu_init_vcpu(), it also unblocks [5].
>
> With minor fixes included mentioned during review
>
> Acked-by: Igor Mammedov <imammedo@redhat.com>

Appreciated for your Ack, Igor!

There is a v2[*], could you take a look at that?

[*] 
https://lore.kernel.org/qemu-devel/20241219110125.1266461-1-xiaoyao.li@intel.com/

> > [1] 
> > https://lore.kernel.org/qemu-devel/20241125103857.78a23715@imammedo.users.ipa.redhat.com/
> > [2] 
> > https://lore.kernel.org/qemu-devel/5f8db586-cdda-4d00-be02-f9880a20e1a3@redhat.com/
> > [3] 
> > https://lore.kernel.org/qemu-devel/20241122160317.4070177-1-xiaoyao.li@intel.com/
> > [4] 
> > https://lore.kernel.org/qemu-devel/045f9cb1-2b17-4b2c-985f-3c34e3626b36@intel.com/
> > [5] 
> > https://lore.kernel.org/qemu-devel/20231128171239.69b6d7b1@imammedo.users.ipa.redhat.com/
> >
> > Xiaoyao Li (4):
> >    i386/topology: Update the comment of x86_apicid_from_topo_ids()
> >    i386: Extract a common fucntion to setup value of
> >      MSR_CORE_THREAD_COUNT
> >    i386: Track cores_per_module in CPUX86State
> >    cpu: Remove nr_cores from struct CPUState
> >
> >   hw/core/cpu-common.c                 |  1 -
> >   hw/i386/x86-common.c                 |  4 +++-
> >   include/hw/core/cpu.h                |  2 --
> >   include/hw/i386/topology.h           |  5 +++--
> >   system/cpus.c                        |  1 -
> >   target/i386/cpu.c                    |  2 +-
> >   target/i386/cpu.h                    | 16 ++++++++++++++++
> >   target/i386/hvf/x86_emu.c            |  3 +--
> >   target/i386/kvm/kvm.c                |  5 +----
> >   target/i386/tcg/sysemu/misc_helper.c |  3 +--
> >   10 files changed, 26 insertions(+), 16 deletions(-)