[PATCH] Don't race while creating a VCPU

[PATCH] Don't race while creating a VCPU

[Anthony Liguori]

We hold qemu_mutex while machine->init() executes, which issues a VCPU create.
We need to make sure to not return from the VCPU creation until the VCPU
file descriptor is valid to ensure that APIC creation succeeds.

However, we also need to make sure that the VCPU thread doesn't start running
until the machine->init() is complete.  This is addressed today because the
VCPU thread tries to grab the qemu_mutex before doing anything interesting.
If we release qemu_mutex to wait for VCPU creation, then we open a window for
a race to occur.

This patch introduces two wait conditions.  The first lets the VCPU create
code that runs in the IO thread to wait for a VCPU to initialize.  The second
condition lets the VCPU thread wait for the machine to fully initialize before
running.

An added benefit of this patch is it makes the dependencies now explicit.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 78127de..9a9bf59 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -32,8 +32,12 @@ static int qemu_kvm_reset_requested;
 
 pthread_mutex_t qemu_mutex = PTHREAD_MUTEX_INITIALIZER;
 pthread_cond_t qemu_aio_cond = PTHREAD_COND_INITIALIZER;
+pthread_cond_t qemu_vcpu_cond = PTHREAD_COND_INITIALIZER;
+pthread_cond_t qemu_system_cond = PTHREAD_COND_INITIALIZER;
 __thread struct vcpu_info *vcpu;
 
+static int qemu_system_ready;
+
 struct qemu_kvm_signal_table {
     sigset_t sigset;
     sigset_t negsigset;
@@ -53,6 +57,7 @@ struct vcpu_info {
     int stop;
     int stopped;
     int reload_regs;
+    int created;
 } vcpu_info[256];
 
 pthread_t io_thread;
@@ -324,6 +329,7 @@ static int kvm_main_loop_cpu(CPUState *env)
     struct vcpu_info *info = &vcpu_info[env->cpu_index];
 
     setup_kernel_sigmask(env);
+
     pthread_mutex_lock(&qemu_mutex);
     if (kvm_irqchip_in_kernel(kvm_context))
 	env->hflags &= ~HF_HALTED_MASK;
@@ -370,6 +376,17 @@ static void *ap_main_loop(void *_env)
     sigprocmask(SIG_BLOCK, &signals, NULL);
     kvm_create_vcpu(kvm_context, env->cpu_index);
     kvm_qemu_init_env(env);
+
+    /* signal VCPU creation */
+    pthread_mutex_lock(&qemu_mutex);
+    vcpu->created = 1;
+    pthread_cond_signal(&qemu_vcpu_cond);
+
+    /* and wait for machine initialization */
+    while (!qemu_system_ready)
+	pthread_cond_wait(&qemu_system_cond, &qemu_mutex);
+    pthread_mutex_unlock(&qemu_mutex);
+
     kvm_main_loop_cpu(env);
     return NULL;
 }
@@ -389,8 +406,9 @@ static void kvm_add_signal(struct qemu_kvm_signal_table *sigtab, int signum)
 void kvm_init_new_ap(int cpu, CPUState *env)
 {
     pthread_create(&vcpu_info[cpu].thread, NULL, ap_main_loop, env);
-    /* FIXME: wait for thread to spin up */
-    usleep(200);
+
+    while (vcpu_info[cpu].created == 0)
+	pthread_cond_wait(&qemu_vcpu_cond, &qemu_mutex);
 }
 
 static void qemu_kvm_init_signal_tables(void)
@@ -435,7 +453,11 @@ void qemu_kvm_notify_work(void)
 int kvm_main_loop(void)
 {
     io_thread = pthread_self();
+    qemu_system_ready = 1;
     pthread_mutex_unlock(&qemu_mutex);
+
+    pthread_cond_broadcast(&qemu_system_cond);
+
     while (1) {
         kvm_eat_signal(&io_signal_table, NULL, 1000);
         pthread_mutex_lock(&qemu_mutex);

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] Don't race while creating a VCPU

[Ryan Harper]

* Anthony Liguori <aliguori@us.ibm.com> [2008-04-28 17:30]:
> We hold qemu_mutex while machine->init() executes, which issues a VCPU create.
> We need to make sure to not return from the VCPU creation until the VCPU
> file descriptor is valid to ensure that APIC creation succeeds.
> 
> However, we also need to make sure that the VCPU thread doesn't start running
> until the machine->init() is complete.  This is addressed today because the
> VCPU thread tries to grab the qemu_mutex before doing anything interesting.
> If we release qemu_mutex to wait for VCPU creation, then we open a window for
> a race to occur.
> 
> This patch introduces two wait conditions.  The first lets the VCPU create
> code that runs in the IO thread to wait for a VCPU to initialize.  The second
> condition lets the VCPU thread wait for the machine to fully initialize before
> running.
> 
> An added benefit of this patch is it makes the dependencies now explicit.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

This patch passed the same tests I used on the other: 64 1VCPU guest
launch, 1 second apart, and a 16-way SMP guest boot.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH] Don't race while creating a VCPU

[Avi Kivity]

Anthony Liguori wrote:
> We hold qemu_mutex while machine->init() executes, which issues a VCPU create.
> We need to make sure to not return from the VCPU creation until the VCPU
> file descriptor is valid to ensure that APIC creation succeeds.
>
> However, we also need to make sure that the VCPU thread doesn't start running
> until the machine->init() is complete.  This is addressed today because the
> VCPU thread tries to grab the qemu_mutex before doing anything interesting.
> If we release qemu_mutex to wait for VCPU creation, then we open a window for
> a race to occur.
>
> This patch introduces two wait conditions.  The first lets the VCPU create
> code that runs in the IO thread to wait for a VCPU to initialize.  The second
> condition lets the VCPU thread wait for the machine to fully initialize before
> running.
>
>   

Applied, thanks.


> An added benefit of this patch is it makes the dependencies now explicit.
>
>   

Indeed.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone