On 5/10/25 10:51, Akihiko Odaki wrote:
Add special implementations of qemu_event_set() and qemu_event_wait()
using pthread primitives. qemu_event_wait() will ensure qemu_event_set()
finishes, and these functions will avoid complex barrier and atomic
operations.
Unfortunately not...
Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
Tested-by: Phil Dennis-Jordan <p...@philjordan.eu>
Reviewed-by: Phil Dennis-Jordan <p...@philjordan.eu>
---
util/qemu-thread-posix.c | 45 ++++++++++++++++++---------------------------
1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 13459e44c768..805cac444f15 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -319,28 +319,6 @@ void qemu_sem_wait(QemuSemaphore *sem)
#ifdef __linux__
#include "qemu/futex.h"
-#else
-static inline void qemu_futex_wake(QemuEvent *ev, int n)
-{
- assert(ev->initialized);
- pthread_mutex_lock(&ev->lock);
- if (n == 1) {
- pthread_cond_signal(&ev->cond);
- } else {
- pthread_cond_broadcast(&ev->cond);
- }
- pthread_mutex_unlock(&ev->lock);
-}
-
-static inline void qemu_futex_wait(QemuEvent *ev, unsigned val)
-{
- assert(ev->initialized);
- pthread_mutex_lock(&ev->lock);
- if (ev->value == val) {
- pthread_cond_wait(&ev->cond, &ev->lock);
- }
- pthread_mutex_unlock(&ev->lock);
-}
#endif
/* Valid transitions:
@@ -363,7 +341,7 @@ static inline void qemu_futex_wait(QemuEvent *ev, unsigned
val)
void qemu_event_init(QemuEvent *ev, bool init)
{
-#ifndef __linux__
+#ifndef CONFIG_LINUX
pthread_mutex_init(&ev->lock, NULL);
pthread_cond_init(&ev->cond, NULL);
#endif
@@ -376,7 +354,7 @@ void qemu_event_destroy(QemuEvent *ev)
{
assert(ev->initialized);
ev->initialized = false;
-#ifndef __linux__
+#ifndef CONFIG_LINUX
pthread_mutex_destroy(&ev->lock);
pthread_cond_destroy(&ev->cond);
#endif
@@ -386,6 +364,7 @@ void qemu_event_set(QemuEvent *ev)
{
assert(ev->initialized);
+#ifdef CONFIG_LINUX
/*
* Pairs with both qemu_event_reset() and qemu_event_wait().
*
The user of this code will not have mutexes so some care is needed. You
have:
if (!qatomic_load_acquire(&done)) {
qemu_event_reset(&ev);
if (!qatomic_load_acquire(&done))
qemu_event_wait(&ev);
}
and on the other side
qatomic_store_release(&done, 1);
qemu_event_set(&ev);
--> qatomic_set(&ev.value, EV_SET);
I don't think this is correct without the memory barrier in
qemu_event_set(), though I am not actually sure how you'd
add it.
The problem is, I don't see anything that prevents this:
set done
qemu_event_set()
pthread_mutex_lock()
ev.value = SET
qemu_event_reset()
ev.value |= FREE
smp_mb__after_rmw()
// store buffer not flushed yet,
// so other thread doesn't see "done"
read done
pthread_mutex_lock()
pthread_cond_broadcast()
pthread_mutex_unlock()
while ev.value != SET
// hang
pthread_cond_wait()
The barrier in qemu_event_reset() is not enough, you need one on each side.
+#else
+ pthread_mutex_lock(&ev->lock);
+ if (qatomic_read(&ev->value) != EV_SET) {
Apart from the above this needs to be a while(), because condition
variables also have spurious wakeups.
Paolo
+ pthread_cond_wait(&ev->cond, &ev->lock);
+ }
+ pthread_mutex_unlock(&ev->lock);
+#endif
}
static __thread NotifierList thread_exit;
