Re: [PATCH] Fix QEMU vcpu thread race with apic_reset

[Avi Kivity <avi@qumranet.com>]

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