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

[Zhenyu Ye <yezhenyu2@huawei.com>]

Hi Eduardo,

Sorry for the delay.

On 2020/12/22 5:36, Eduardo Habkost wrote:
> 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().
> 

There's a chance that this happens.
Could we move these (after qemu_init_vcpu()) out of x86_cpu_realizefn()
to the x86_cpus_init(), after qemu_wait_all_vcpu_threads_init()?
Such as:

void x86_cpus_init()
{
	foreach (cpu) {
		x86_cpu_new();
	}

	qemu_wait_all_vcpu_threads_init();

	foreach (cpu) {
		x86_cpu_new_post();
	}
}

> 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?
> 

The most time-consuming operation in haxm is ioctl(HAX_VM_IOCTL_VCPU_CREATE).
Saddly this can not be split.

Even if we fix the problem in haxm, other accelerators may also have
this problem.  So I think if we can make the x86_cpu_new() concurrently,
we should try to do it.

Thanks,
Zhenyu