| Message ID | 20190306115532.23025-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iothread: create gcontext unconditionally | expand |
On Wed, Mar 06, 2019 at 07:55:31PM +0800, Peter Xu wrote: > + /* > + * We should do this as soon as we enter the thread, because the > + * function will silently fail if it fails to acquire the > + * gcontext. > + */ > + g_main_context_push_thread_default(iothread->worker_context); I have a hard time understanding this comment. The mention of how it fails makes me think "we'll never find out about failures anyway, so how does it help to call this early?". I suggest sticking to the point that this function must always be called first: /* * g_main_context_push_thread_default() must be called before anything * in this new thread uses glib. */ Now people will think before moving this function call. Stefan
On Thu, Mar 07, 2019 at 02:40:39PM +0000, Stefan Hajnoczi wrote: > On Wed, Mar 06, 2019 at 07:55:31PM +0800, Peter Xu wrote: > > + /* > > + * We should do this as soon as we enter the thread, because the > > + * function will silently fail if it fails to acquire the > > + * gcontext. > > + */ > > + g_main_context_push_thread_default(iothread->worker_context); > > I have a hard time understanding this comment. The mention of how it > fails makes me think "we'll never find out about failures anyway, so how > does it help to call this early?". > > I suggest sticking to the point that this function must always be called > first: > > /* > * g_main_context_push_thread_default() must be called before anything > * in this new thread uses glib. > */ > > Now people will think before moving this function call. Sorry to be confusing; this looks good to me. Please let me know if you want me to repost this patch alone or the patchset with the change. Thanks,
diff --git a/iothread.c b/iothread.c index 9abdbace66..045825a348 100644 --- a/iothread.c +++ b/iothread.c @@ -53,7 +53,12 @@ static void *iothread_run(void *opaque) IOThread *iothread = opaque; rcu_register_thread(); - + /* + * We should do this as soon as we enter the thread, because the + * function will silently fail if it fails to acquire the + * gcontext. + */ + g_main_context_push_thread_default(iothread->worker_context); my_iothread = iothread; iothread->thread_id = qemu_get_thread_id(); qemu_sem_post(&iothread->init_done_sem); @@ -66,12 +71,11 @@ static void *iothread_run(void *opaque) * changed in previous aio_poll() */ if (iothread->running && atomic_read(&iothread->run_gcontext)) { - g_main_context_push_thread_default(iothread->worker_context); g_main_loop_run(iothread->main_loop); - g_main_context_pop_thread_default(iothread->worker_context); } } + g_main_context_pop_thread_default(iothread->worker_context); rcu_unregister_thread(); return NULL; }
We were pushing the context until right before running the gmainloop. Now since we have everything unconditionally, we can move this earlier. One benefit is that now it's done even before init_done_sem, so as long as the iothread user calls iothread_create() and completes, we know that the thread stack is ready. Signed-off-by: Peter Xu <peterx@redhat.com> --- iothread.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)