Re: [PATCH] replay: implement fair mutex

[Pavel Dovgalyuk <dovgaluk@ispras.ru>]

On 18.05.2020 19:07, Alex Bennée wrote:
> Pavel Dovgalyuk <Pavel.Dovgaluk@gmail.com> writes:
>
>> In record/replay icount mode main loop thread and vCPU thread
>> do not perform simultaneously. They take replay mutex to synchronize
>> the actions. Sometimes vCPU thread waits for locking the mutex for
>> very long time, because main loop releases the mutex and takes it
>> back again.
> Where in the main loop do we keep bouncing the mutex like this? Surely
> that is the problem we should fix?

I performed kind of profilng while replaying.

Sometimes main loop takes and releases this mutex without giving a 
chance to vCPU to work.

I also got reports about the opposite behavior from the users: vCPU 
takes and releases the mutex, and main loop stalls.

>
>> Standard qemu mutex do not provide the ordering
>> capabilities.
>>
>> This patch adds a "queue" for replay mutex. Therefore thread ordering
>> becomes more "fair". Threads are executed in the same order as
>> they are trying to take the mutex.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>> ---
>>   replay/replay-internal.c |   15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c
>> index eba8246aae..2e8a3e947a 100644
>> --- a/replay/replay-internal.c
>> +++ b/replay/replay-internal.c
>> @@ -22,6 +22,9 @@
>>      It also protects replay events queue which stores events to be
>>      written or read to the log. */
>>   static QemuMutex lock;
>> +/* Condition and queue for fair ordering of mutex lock requests. */
>> +static QemuCond mutex_cond;
>> +static unsigned long mutex_head, mutex_tail;
>>   
>>   /* File for replay writing */
>>   static bool write_error;
>> @@ -197,9 +200,10 @@ static __thread bool replay_locked;
>>   void replay_mutex_init(void)
>>   {
>>       qemu_mutex_init(&lock);
>> +    qemu_cond_init(&mutex_cond);
>>       /* Hold the mutex while we start-up */
>> -    qemu_mutex_lock(&lock);
>>       replay_locked = true;
>> +    ++mutex_tail;
>>   }
>>   
>>   bool replay_mutex_locked(void)
>> @@ -211,10 +215,16 @@ bool replay_mutex_locked(void)
>>   void replay_mutex_lock(void)
>>   {
>>       if (replay_mode != REPLAY_MODE_NONE) {
>> +        unsigned long id;
>>           g_assert(!qemu_mutex_iothread_locked());
>>           g_assert(!replay_mutex_locked());
>>           qemu_mutex_lock(&lock);
>> +        id = mutex_tail++;
>> +        while (id != mutex_head) {
>> +            qemu_cond_wait(&mutex_cond, &lock);
>> +        }
>>           replay_locked = true;
>> +        qemu_mutex_unlock(&lock);
>>       }
>>   }
>>   
>> @@ -222,7 +232,10 @@ void replay_mutex_unlock(void)
>>   {
>>       if (replay_mode != REPLAY_MODE_NONE) {
>>           g_assert(replay_mutex_locked());
>> +        qemu_mutex_lock(&lock);
>> +        ++mutex_head;
>>           replay_locked = false;
>> +        qemu_cond_broadcast(&mutex_cond);
>>           qemu_mutex_unlock(&lock);
>>       }
>>   }
>