Re: [RFC PATCH v2] x86/cpu: initialize the CPU concurrently

[Eduardo Habkost]

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 <ehabkost@redhat.com>
> Signed-off-by: eillon <yezhenyu2@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