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

[Anthony Liguori <anthony@codemonkey.ws>]

Ryan Harper wrote:
> * Avi Kivity <avi@qumranet.com> [2008-04-26 02:23]:
>   
>> Please reuse qemu_mutex for this, no need for a new one.
>>     
>
> I'm having a little trouble wrapping my head around all of the locking
> here.  If I avoid qemu_mutex and use a new one, I've got everything
> working.  However, attemping to use qemu_mutex is stepping into a pile
> of locking crap.  I'm sure there is a good reason... 
>
> The current code looks like this:
>
> Thread1:
> main()
>   kvm_qemu_init() // mutex_lock()
>   machine->init()
>    pc_init1()
>     pc_new_cpu()
>      cpu_init()
>       cpu_x86_init()
>        kvm_init_new_ap() // create vcpu Thread2
>        <--      
>       <--      
>      <--      
>     <--      
>    <--      
>   <--      
>   kvm_main_loop() // mutex_unlock()
>                         
> Thread2:
> ap_main_loop()
>  /* vcpu init */
>  kvm_main_loop_cpu()
>   kvm_main_loop_wait() // mutex_unlock() on enter, lock on exit
>    kvm_eat_signals()   // mutex_lock() on enter, unlock on exit
>    <--
>   <--
>  <--
>   

The qemu_mutex is meant to ensure that the QEMU code is only ever 
entered by one thread at a time.  QEMU is not thread-safe so this is 
necessary.  It's a little odd because we're taking this lock initially 
by being called from QEMU code.  Here's the basic theory of locking AFAICT:

kvm_main_loop_cpu() is the main loop for each VCPU.  It must acquire the 
QEMU mutex since it will call into normal QEMU code to process events.  
Whenever a VCPU is allowed to run, or when the VCPU is idling, it needs 
to release the QEMU mutex.  The former is done via the post_kvm_run() 
and pre_kvm_run() hooks.  The later is done within kvm_main_loop_wait().

kvm_main_loop_wait() will release the QEMU mutex and call 
kvm_eat_signals() which calls kvm_eat_signal() which will issue a 
sigtimedwait().  This is where we actually idle (and why SIGIO is so 
important right now).  We don't want to idle with the QEMU mutex held as 
that may result in dead lock so this is why we release it here.

kvm_eat_signal() has to acquire the lock again in order to dispatch IO 
events (via kvm_process_signal()).

Regards,

Anthony Liguori

> It wedges up in kvm_init_new_ap() if I attempt acquire qemu_mutex.
> Quite obvious after I looked at the call trace and discovered
> kvm_qemu_init() locking on exit.  I see other various functions that
> unlock and then lock; I really don't want to wade into this mess...
> rather whomever cooked it up should do some cleanup.  I tried the
> unlock, then re-lock on exit in kvm_init_new_ap() but that also wedged.
>
> Here is a rework with a new flag in vcpu_info indicating vcpu
> creation.  Tested this with 64 1VCPU guests booting with 1 second delay,
> and single 16-way SMP guest boot.
>
>   


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