On Mon, Dec 21, 2020 at 07:36:18PM +0800, Zhenyu Ye wrote:
> Providing a optional mechanism to wait for all VCPU threads be
> created out of qemu_init_vcpu(), then we can initialize the cpu
> concurrently on the x86 architecture.
>
> This reduces the time of creating virtual machines. For example, when
> the haxm is used as the accelerator, cpus_accel->create_vcpu_thread()
> will cause at least 200ms for each cpu, extremely prolong the boot
> time.
>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> Signed-off-by: eillon <yezhen...@huawei.com>
The patch is easier to follow now, but I have a question that may
be difficult to answer:
What exactly is the meaning of cpu->created=true, and what
exactly would break if we never wait for cpu->created==true at all?
I'm asking that because we might be introducing subtle races
here, if some of the remaining CPU initialization code in
x86_cpu_realizefn() [1] expects the VCPU thread to be already
initialized.
The cpu_reset() call below is one such example (but probably not
the only one). cpu_reset() ends up calling
kvm_arch_reset_vcpu(), which seems to assume kvm_init_vcpu() was
already called. With your patch, kvm_init_vcpu() might end up
being called after kvm_arch_reset_vcpu().
Maybe a simpler alternative is to keep the existing thread
creation logic, but changing hax_cpu_thread_fn() to do less work
before calling cpu_thread_signal_created()?
In my testing (without this patch), creation of 8 KVM VCPU
threads in a 4 core machine takes less than 3 ms. Why is
qemu_init_vcpu() taking so long on haxm? Which parts of haxm
initialization can be moved after cpu_thread_signal_created(), to
make this better?
---
[1] For reference, the last few lines of x86_cpu_realizefn() are:
| qemu_init_vcpu(cs);
|
| /*
| * Most Intel and certain AMD CPUs support hyperthreading. Even though
QEMU
| * fixes this issue by adjusting CPUID_0000_0001_EBX and
CPUID_8000_0008_ECX
| * based on inputs (sockets,cores,threads), it is still better to give
| * users a warning.
| *
| * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
| * cs->nr_threads hasn't be populated yet and the checking is incorrect.
| */
| if (IS_AMD_CPU(env) &&
| !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
| cs->nr_threads > 1 && !ht_warned) {
| warn_report("This family of AMD CPU doesn't support "
| "hyperthreading(%d)",
| cs->nr_threads);
| error_printf("Please configure -smp options properly"
| " or try enabling topoext feature.\n");
| ht_warned = true;
| }
|
| x86_cpu_apic_realize(cpu, &local_err);
| if (local_err != NULL) {
| goto out;
| }
| cpu_reset(cs);
|
| xcc->parent_realize(dev, &local_err);
|
| out:
| if (local_err != NULL) {
| error_propagate(errp, local_err);
| return;
| }
| }
> ---
> hw/i386/x86.c | 3 +++
> include/hw/core/cpu.h | 13 +++++++++++++
> softmmu/cpus.c | 21 +++++++++++++++++++--
> 3 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 6329f90ef9..09afff724a 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -108,6 +108,8 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id,
> Error **errp)
> if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
> goto out;
> }
> +
> + CPU(cpu)->async_init = true;
> qdev_realize(DEVICE(cpu), NULL, errp);
>
> out:
> @@ -137,6 +139,7 @@ void x86_cpus_init(X86MachineState *x86ms, int
> default_cpu_version)
> for (i = 0; i < ms->smp.cpus; i++) {
> x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
> }
> + qemu_wait_all_vcpu_threads_init();
> }
>
> void x86_rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 8e7552910d..55c2c17d93 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -467,6 +467,12 @@ struct CPUState {
>
> /* track IOMMUs whose translations we've cached in the TCG TLB */
> GArray *iommu_notifiers;
> +
> + /*
> + * If true, qemu_init_vcpu() will not wait for the VCPU thread to be
> created
> + * before returning.
> + */
> + bool async_init;
> };
>
> typedef QTAILQ_HEAD(CPUTailQ, CPUState) CPUTailQ;
> @@ -977,6 +983,13 @@ void start_exclusive(void);
> */
> void end_exclusive(void);
>
> +/**
> + * qemu_wait_all_vcpu_threads_init:
> + *
> + * Wait for all VCPU threads to be created.
> + */
> +void qemu_wait_all_vcpu_threads_init(void);
> +
> /**
> * qemu_init_vcpu:
> * @cpu: The vCPU to initialize.
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 1dc20b9dc3..d76853d356 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -601,6 +601,23 @@ void cpus_register_accel(const CpusAccel *ca)
> cpus_accel = ca;
> }
>
> +static void qemu_wait_vcpu_thread_init(CPUState *cpu)
> +{
> + while (!cpu->created) {
> + qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> + }
> +}
> +
> +void qemu_wait_all_vcpu_threads_init(void)
> +{
> + CPUState *cpu;
> +
> + CPU_FOREACH(cpu) {
> + printf("***** cpuid: %d\n", cpu->cpu_index);
Debugging leftover.
> + qemu_wait_vcpu_thread_init(cpu);
> + }
> +}
> +
> void qemu_init_vcpu(CPUState *cpu)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> @@ -622,8 +639,8 @@ void qemu_init_vcpu(CPUState *cpu)
> g_assert(cpus_accel != NULL && cpus_accel->create_vcpu_thread != NULL);
> cpus_accel->create_vcpu_thread(cpu);
>
> - while (!cpu->created) {
> - qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
> + if (!cpu->async_init) {
> + qemu_wait_vcpu_thread_init(cpu);
> }
> }
>
> --
> 2.22.0.windows.1
>
--
Eduardo
