Re: Role of qemu_fair_mutex

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/04/2011 09:12 AM, Avi Kivity wrote:
> On 01/04/2011 04:55 PM, Anthony Liguori wrote:
>>
>>>>
>>>> When the TCG thread, it needs to let the IO thread run for at least 
>>>> one iteration.  Coordinating the execution of the IO thread such 
>>>> that it's guaranteed to run at least once and then having it drop 
>>>> the qemu mutex long enough for the TCG thread to acquire it is the 
>>>> purpose of the qemu_fair_mutex.
>>>
>>> That doesn't compute - the iothread doesn't hog the global lock (it 
>>> sleeps most of the time, and drops the lock while sleeping), so the 
>>> iothread cannot starve out tcg.
>>
>> The fact that the iothread drops the global lock during sleep is a 
>> detail that shouldn't affect correctness.  The IO thread is 
>> absolutely allowed to run for arbitrary periods of time without 
>> dropping the qemu mutex.
>
> No, it's not, since it will stop vcpus in their tracks.  Whenever we 
> hold qemu_mutex for unbounded time, that's a bug.

I'm not sure that designing the io thread to hold the lock for a 
"bounded" amount of time is a good design point.  What is an accepted 
amount of time for it to hold the lock?

Instead of the VCPU relying on the IO thread to eventually drop the 
lock, it seems far superior to have the VCPU thread indicate to the IO 
thread that it needs the lock.

As of right now, the IO thread can indicate to the VCPU thread that it 
needs the lock so having a symmetric interface seems obvious.  Of 
course, you need to pick one to have more priority in case both indicate 
they need to use the lock at the same exact time.

>   I think the only place is live migration and perhaps tcg?

qcow2 and anything else that puts the IO thread to sleep.

>>>   On the other hand, tcg does hog the global lock, so it needs to be 
>>> made to give it up so the iothread can run, for example my 
>>> completion example.
>>
>> It's very easy to ask TCG to give up the qemu_mutex by using 
>> cpu_interrupt().  It will drop the qemu_mutex and it will not attempt 
>> to acquire it again until you restart the VCPU.
>
> Maybe that's the solution:
>
> def acquire_global_mutex():
>    if not tcg_thread:
>       cpu_interrupt()

It's not quite as direct as this at the moment but this is also not 
really a bad idea.  Right now we just send a SIG_IPI but cpu_interrupt 
would be better.

>    global_mutex.aquire()
>
> release_global_mutex():
>     global_mutex.release()
>     if not tcg_thread:
>        cpu_resume()
>
> though it's racy, two non-tcg threads can cause an early resume.
>
>>
>>> I think the abstraction we need here is a priority lock, with higher 
>>> priority given to the iothread.  A lock() operation that takes 
>>> precedence would atomically signal the current owner to drop the lock.
>>
>> The I/O thread can reliably acquire the lock whenever it needs to.
>>
>> If you drop all of the qemu_fair_mutex stuff and leave the qemu_mutex 
>> getting dropped around select, TCG will generally work reliably.  But 
>> this is not race free. 
>
> What would be the impact of a race here?

Racy is probably the wrong word.  To give a concrete example of why one 
is better than the other, consider live migration.

It would be reasonable to have a check in live migration to iterate 
unless there was higher priority work.  If a VCPU thread needs to 
acquire the mutex, that could be considered higher priority work.  If 
you don't have an explicit hand off, it's not possible to implement such 
logic.

>> Just dropping a lock does not result in reliable hand off.
>
> Why do we want a handoff in the first place?
>
> I don't think we do.  I think we want the iothread to run in 
> preference to tcg, since tcg is a lock hog under guest control, while 
> the iothread is not a lock hog (excepting live migration).

The io thread is a lock hog practically speaking.

Regards,

Anthony Liguori

>>
>> I think a generational counter could work and a condition could work.
>