Pavel Dovgalyuk <dovga...@ispras.ru> writes:
>> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
>> On 31/10/2017 12:26, Pavel Dovgalyuk wrote:
>> > + /* We need to drop the replay_lock so any vCPU threads woken up
>> > + * can finish their replay tasks
>> > + */
>> > + if (replay_mode != REPLAY_MODE_NONE) {
>> > + g_assert(replay_mutex_locked());
>> > + qemu_mutex_unlock_iothread();
>> > + replay_mutex_unlock();
>> > + qemu_mutex_lock_iothread();
>> > + }
>>
>> The assert+unlock+lock here is unnecessary; just do
>>
>> if (replay_mode != REPLAY_MODE_NONE) {
>> replay_mutex_unlock();
>> }
>>
>> which according to a previous suggestion can become just
>>
>> replay_mutex_unlock();
>
> We can't remove qemu_mutex_unlock_iothread(), because there is an assert
> inside replay_mutex_unlock(), which checks that iothread is unlocked.
I'm certainly open to reviewing the locking order rules if it is easier
another way around. I'm just conscious that it's easy to deadlock if we
don't pay attention. This is what I wrote in replay.txt:
Locking and thread synchronisation
----------------------------------
Previously the synchronisation of the main thread and the vCPU thread
was ensured by the holding of the BQL. However the trend has been to
reduce the time the BQL was held across the system including under TCG
system emulation. As it is important that batches of events are kept
in sequence (e.g. expiring timers and checkpoints in the main thread
while instruction checkpoints are written by the vCPU thread) we need
another lock to keep things in lock-step. This role is now handled by
the replay_mutex_lock. It used to be held only for each event being
written but now it is held for a whole execution period. This results
in a deterministic ping-pong between the two main threads.
As deadlocks are easy to introduce a new rule is introduced that the
replay_mutex_lock is taken before any BQL locks. Conversely you cannot
release the replay_lock while the BQL is still held.
>
>>
>> > while (!all_vcpus_paused()) {
>> > qemu_cond_wait(&qemu_pause_cond, &qemu_global_mutex);
>> > CPU_FOREACH(cpu) {
>> > qemu_cpu_kick(cpu);
>> > }
>> > }
>> > +
>> > + if (replay_mode != REPLAY_MODE_NONE) {
>> > + qemu_mutex_unlock_iothread();
>> > + replay_mutex_lock();
>> > + qemu_mutex_lock_iothread();
>> > + }
>>
>
> Pavel Dovgalyuk
--
Alex Bennée
