* [PATCH] replay: implement fair mutex
@ 2020-04-30 9:46 Pavel Dovgalyuk
2020-05-18 16:07 ` Alex Bennée
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Dovgalyuk @ 2020-04-30 9:46 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, dovgaluk, pavel.dovgaluk
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. 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);
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] replay: implement fair mutex
2020-04-30 9:46 [PATCH] replay: implement fair mutex Pavel Dovgalyuk
@ 2020-05-18 16:07 ` Alex Bennée
2020-05-19 5:46 ` Pavel Dovgalyuk
0 siblings, 1 reply; 5+ messages in thread
From: Alex Bennée @ 2020-05-18 16:07 UTC (permalink / raw)
To: Pavel Dovgalyuk; +Cc: pbonzini, dovgaluk, qemu-devel, pavel.dovgaluk
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?
> 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);
> }
> }
--
Alex Bennée
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] replay: implement fair mutex
2020-05-18 16:07 ` Alex Bennée
@ 2020-05-19 5:46 ` Pavel Dovgalyuk
0 siblings, 0 replies; 5+ messages in thread
From: Pavel Dovgalyuk @ 2020-05-19 5:46 UTC (permalink / raw)
To: Alex Bennée, Pavel Dovgalyuk; +Cc: pbonzini, qemu-devel, pavel.dovgaluk
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);
>> }
>> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] replay: implement fair mutex
@ 2020-04-30 9:13 Pavel Dovgalyuk
2020-04-30 9:30 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Dovgalyuk @ 2020-04-30 9:13 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, dovgaluk, pavel.dovgaluk
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. 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);
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] replay: implement fair mutex
2020-04-30 9:13 Pavel Dovgalyuk
@ 2020-04-30 9:30 ` Paolo Bonzini
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2020-04-30 9:30 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel; +Cc: dovgaluk, pavel.dovgaluk
On 30/04/20 11:13, Pavel Dovgalyuk wrote:
> 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. 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);
> }
> }
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-05-19 5:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-30 9:46 [PATCH] replay: implement fair mutex Pavel Dovgalyuk
2020-05-18 16:07 ` Alex Bennée
2020-05-19 5:46 ` Pavel Dovgalyuk
-- strict thread matches above, loose matches on Subject: below --
2020-04-30 9:13 Pavel Dovgalyuk
2020-04-30 9:30 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.