Re: [Qemu-devel] [RFC PATCH 17/26] replay: push replay_mutex_lock up the call tree

[Alex Bennée]

Pavel Dovgalyuk <address@hidden> writes:

>> From: Paolo Bonzini [mailto:address@hidden
>> 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