Only sending an init-done message using lock+cond seems an overkill to
me. Replacing it with a simpler semaphore.
Meanwhile, init the semaphore unconditionally, then we can destroy it
unconditionally too in finalize which seems cleaner.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/sysemu/iothread.h | 3 +--
iothread.c | 17 ++++-------------
2 files changed, 5 insertions(+), 15 deletions(-)
diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index 8a7ac2c528..50411ba54a 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -27,8 +27,7 @@ typedef struct {
GMainContext *worker_context;
GMainLoop *main_loop;
GOnce once;
- QemuMutex init_done_lock;
- QemuCond init_done_cond; /* is thread initialization done? */
+ QemuSemaphore init_done_sem; /* is thread init done? */
bool stopping; /* has iothread_stop() been called? */
bool running; /* should iothread_run() continue? */
int thread_id;
diff --git a/iothread.c b/iothread.c
index e615b7ae52..6e297e9ef1 100644
--- a/iothread.c
+++ b/iothread.c
@@ -55,10 +55,8 @@ static void *iothread_run(void *opaque)
rcu_register_thread();
my_iothread = iothread;
- qemu_mutex_lock(&iothread->init_done_lock);
iothread->thread_id = qemu_get_thread_id();
- qemu_cond_signal(&iothread->init_done_cond);
- qemu_mutex_unlock(&iothread->init_done_lock);
+ qemu_sem_post(&iothread->init_done_sem);
while (iothread->running) {
aio_poll(iothread->ctx, true);
@@ -115,6 +113,7 @@ static void iothread_instance_init(Object *obj)
iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT;
iothread->thread_id = -1;
+ qemu_sem_init(&iothread->init_done_sem, 0);
}
static void iothread_instance_finalize(Object *obj)
@@ -123,10 +122,6 @@ static void iothread_instance_finalize(Object *obj)
iothread_stop(iothread);
- if (iothread->thread_id != -1) {
- qemu_cond_destroy(&iothread->init_done_cond);
- qemu_mutex_destroy(&iothread->init_done_lock);
- }
/*
* Before glib2 2.33.10, there is a glib2 bug that GSource context
* pointer may not be cleared even if the context has already been
@@ -145,6 +140,7 @@ static void iothread_instance_finalize(Object *obj)
g_main_context_unref(iothread->worker_context);
iothread->worker_context = NULL;
}
+ qemu_sem_destroy(&iothread->init_done_sem);
}
static void iothread_complete(UserCreatable *obj, Error **errp)
@@ -173,8 +169,6 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
return;
}
- qemu_mutex_init(&iothread->init_done_lock);
- qemu_cond_init(&iothread->init_done_cond);
iothread->once = (GOnce) G_ONCE_INIT;
/* This assumes we are called from a thread with useful CPU affinity for us
@@ -188,12 +182,9 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
g_free(name);
/* Wait for initialization to complete */
- qemu_mutex_lock(&iothread->init_done_lock);
while (iothread->thread_id == -1) {
- qemu_cond_wait(&iothread->init_done_cond,
- &iothread->init_done_lock);
+ qemu_sem_wait(&iothread->init_done_sem);
}
- qemu_mutex_unlock(&iothread->init_done_lock);
}
typedef struct {
--
2.17.1
Hi
On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
>
> Only sending an init-done message using lock+cond seems an overkill to
> me. Replacing it with a simpler semaphore.
>
> Meanwhile, init the semaphore unconditionally, then we can destroy it
> unconditionally too in finalize which seems cleaner.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
The lock is also protecting thread_id.
> ---
> include/sysemu/iothread.h | 3 +--
> iothread.c | 17 ++++-------------
> 2 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> index 8a7ac2c528..50411ba54a 100644
> --- a/include/sysemu/iothread.h
> +++ b/include/sysemu/iothread.h
> @@ -27,8 +27,7 @@ typedef struct {
> GMainContext *worker_context;
> GMainLoop *main_loop;
> GOnce once;
> - QemuMutex init_done_lock;
> - QemuCond init_done_cond; /* is thread initialization done? */
> + QemuSemaphore init_done_sem; /* is thread init done? */
> bool stopping; /* has iothread_stop() been called? */
> bool running; /* should iothread_run() continue? */
> int thread_id;
> diff --git a/iothread.c b/iothread.c
> index e615b7ae52..6e297e9ef1 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -55,10 +55,8 @@ static void *iothread_run(void *opaque)
> rcu_register_thread();
>
> my_iothread = iothread;
> - qemu_mutex_lock(&iothread->init_done_lock);
> iothread->thread_id = qemu_get_thread_id();
> - qemu_cond_signal(&iothread->init_done_cond);
> - qemu_mutex_unlock(&iothread->init_done_lock);
> + qemu_sem_post(&iothread->init_done_sem);
>
> while (iothread->running) {
> aio_poll(iothread->ctx, true);
> @@ -115,6 +113,7 @@ static void iothread_instance_init(Object *obj)
>
> iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT;
> iothread->thread_id = -1;
> + qemu_sem_init(&iothread->init_done_sem, 0);
> }
>
> static void iothread_instance_finalize(Object *obj)
> @@ -123,10 +122,6 @@ static void iothread_instance_finalize(Object *obj)
>
> iothread_stop(iothread);
>
> - if (iothread->thread_id != -1) {
> - qemu_cond_destroy(&iothread->init_done_cond);
> - qemu_mutex_destroy(&iothread->init_done_lock);
> - }
> /*
> * Before glib2 2.33.10, there is a glib2 bug that GSource context
> * pointer may not be cleared even if the context has already been
> @@ -145,6 +140,7 @@ static void iothread_instance_finalize(Object *obj)
> g_main_context_unref(iothread->worker_context);
> iothread->worker_context = NULL;
> }
> + qemu_sem_destroy(&iothread->init_done_sem);
> }
>
> static void iothread_complete(UserCreatable *obj, Error **errp)
> @@ -173,8 +169,6 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
> return;
> }
>
> - qemu_mutex_init(&iothread->init_done_lock);
> - qemu_cond_init(&iothread->init_done_cond);
> iothread->once = (GOnce) G_ONCE_INIT;
>
> /* This assumes we are called from a thread with useful CPU affinity for us
> @@ -188,12 +182,9 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
> g_free(name);
>
> /* Wait for initialization to complete */
> - qemu_mutex_lock(&iothread->init_done_lock);
> while (iothread->thread_id == -1) {
> - qemu_cond_wait(&iothread->init_done_cond,
> - &iothread->init_done_lock);
> + qemu_sem_wait(&iothread->init_done_sem);
> }
> - qemu_mutex_unlock(&iothread->init_done_lock);
> }
>
> typedef struct {
> --
> 2.17.1
>
On Fri, Feb 22, 2019 at 07:25:16AM +0100, Marc-André Lureau wrote: > Hi > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote: > > > > Only sending an init-done message using lock+cond seems an overkill to > > me. Replacing it with a simpler semaphore. > > > > Meanwhile, init the semaphore unconditionally, then we can destroy it > > unconditionally too in finalize which seems cleaner. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > The lock is also protecting thread_id. IMHO it's fine because thread_id is only changed at the beginning of iothread_run where the caller will definitely wait for the thread_id to be generated. Here qemu_sem_post() should at least contain one write memory barrier there to make sure the waker will read the correct value after sem_wait() and then later on thread_id is never changed. Regards, -- Peter Xu
On 22/02/19 07:36, Peter Xu wrote: > On Fri, Feb 22, 2019 at 07:25:16AM +0100, Marc-André Lureau wrote: >> Hi >> >> On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote: >>> >>> Only sending an init-done message using lock+cond seems an overkill to >>> me. Replacing it with a simpler semaphore. >>> >>> Meanwhile, init the semaphore unconditionally, then we can destroy it >>> unconditionally too in finalize which seems cleaner. >>> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >> >> The lock is also protecting thread_id. > > IMHO it's fine because thread_id is only changed at the beginning of > iothread_run where the caller will definitely wait for the thread_id > to be generated. Here qemu_sem_post() should at least contain one > write memory barrier there to make sure the waker will read the > correct value after sem_wait() and then later on thread_id is never > changed. Yes, qemu_sem_post() is a "release" operation. Anything that happens before it is visible to the thread that does qemu_sem_wait(). In fact, thread_id is accessed outside the lock in iothread_instance_finalize, so Peter's change is overall an improvement. Paolo
Hi On Fri, Feb 22, 2019 at 10:33 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 22/02/19 07:36, Peter Xu wrote: > > On Fri, Feb 22, 2019 at 07:25:16AM +0100, Marc-André Lureau wrote: > >> Hi > >> > >> On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote: > >>> > >>> Only sending an init-done message using lock+cond seems an overkill to > >>> me. Replacing it with a simpler semaphore. > >>> > >>> Meanwhile, init the semaphore unconditionally, then we can destroy it > >>> unconditionally too in finalize which seems cleaner. > >>> > >>> Signed-off-by: Peter Xu <peterx@redhat.com> > >> > >> The lock is also protecting thread_id. > > > > IMHO it's fine because thread_id is only changed at the beginning of > > iothread_run where the caller will definitely wait for the thread_id > > to be generated. Here qemu_sem_post() should at least contain one > > write memory barrier there to make sure the waker will read the > > correct value after sem_wait() and then later on thread_id is never > > changed. > > Yes, qemu_sem_post() is a "release" operation. Anything that happens > before it is visible to the thread that does qemu_sem_wait(). ok > In fact, thread_id is accessed outside the lock in > iothread_instance_finalize, so Peter's change is overall an improvement. > finalize() stops the threadm though -- Marc-André Lureau
On Fri, Feb 22, 2019 at 11:14:10AM +0800, Peter Xu wrote: > Only sending an init-done message using lock+cond seems an overkill to > me. Replacing it with a simpler semaphore. > > Meanwhile, init the semaphore unconditionally, then we can destroy it > unconditionally too in finalize which seems cleaner. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/sysemu/iothread.h | 3 +-- > iothread.c | 17 ++++------------- > 2 files changed, 5 insertions(+), 15 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
© 2016 - 2025 Red Hat, Inc.