Re: [PATCH] replay: implement fair mutex

[Pavel Dovgalyuk]

On 18.05.2020 19:07, Alex Bennée wrote:
> Pavel Dovgalyuk <address@hidden> 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 <address@hidden>
> > ---
> >   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);
> >       }
> >   }