From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] Fix QEMU vcpu thread race with apic_reset Date: Sat, 26 Apr 2008 10:16:26 +0300 Message-ID: <4812D6CA.4040407@qumranet.com> References: <1209187574-21081-1-git-send-email-ryanh@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel@lists.sourceforge.net To: Ryan Harper Return-path: In-Reply-To: <1209187574-21081-1-git-send-email-ryanh@us.ibm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces@lists.sourceforge.net Errors-To: kvm-devel-bounces@lists.sourceforge.net List-Id: kvm.vger.kernel.org Ryan Harper wrote: > There is a race between when the vcpu thread issues a create ioctl and when > apic_reset() gets called resulting in getting a badfd error. > > The problem is indeed there, but the fix is wrong: > main thread vcpu thread > diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c > index 78127de..3513e8c 100644 > --- a/qemu/qemu-kvm.c > +++ b/qemu/qemu-kvm.c > @@ -31,7 +31,9 @@ extern int smp_cpus; > static int qemu_kvm_reset_requested; > > pthread_mutex_t qemu_mutex = PTHREAD_MUTEX_INITIALIZER; > +pthread_mutex_t vcpu_mutex = PTHREAD_MUTEX_INITIALIZER; > pthread_cond_t qemu_aio_cond = PTHREAD_COND_INITIALIZER; > +pthread_cond_t qemu_vcpuup_cond = PTHREAD_COND_INITIALIZER; > __thread struct vcpu_info *vcpu; > > struct qemu_kvm_signal_table { > @@ -369,6 +371,11 @@ static void *ap_main_loop(void *_env) > sigfillset(&signals); > sigprocmask(SIG_BLOCK, &signals, NULL); > kvm_create_vcpu(kvm_context, env->cpu_index); > + /* block until cond_wait occurs */ > + pthread_mutex_lock(&vcpu_mutex); > + /* now we can signal */ > + pthread_cond_signal(&qemu_vcpuup_cond); > + pthread_mutex_unlock(&vcpu_mutex); > kvm_qemu_init_env(env); > kvm_main_loop_cpu(env); > return NULL; > @@ -388,9 +395,10 @@ static void kvm_add_signal(struct qemu_kvm_signal_table *sigtab, int signum) > > void kvm_init_new_ap(int cpu, CPUState *env) > { > + pthread_mutex_lock(&vcpu_mutex); > pthread_create(&vcpu_info[cpu].thread, NULL, ap_main_loop, env); > - /* FIXME: wait for thread to spin up */ > - usleep(200); > + pthread_cond_wait(&qemu_vcpuup_cond, &vcpu_mutex); > pthread_cond_wait() is never correct outside a loop. The signal may arrive before wait is called. The usual idiom is while (condition is not fulfilled) pthread_cond_wait(); I see you have something there to ensure we block, but please use the right idiom. > + pthread_mutex_unlock(&vcpu_mutex); > } > Please reuse qemu_mutex for this, no need for a new one. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. ------------------------------------------------------------------------- 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