| Message ID | 20230904144811.4218-1-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | iothread: Set the GSource "name" field | expand |
On 4/9/23 16:48, Fabiano Rosas wrote: > Having a name in the source helps with debugging core dumps when one > might not have access to TLS data to cross-reference AioContexts with > their addresses. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > iothread.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Mon, Sep 04, 2023 at 11:48:11AM -0300, Fabiano Rosas wrote: > @@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp) > return; > } > > + thread_name = g_strdup_printf("IO %s", > + object_get_canonical_path_component(OBJECT(base))); > + > /* > * Init one GMainContext for the iothread unconditionally, even if > * it's not used > */ > - iothread_init_gcontext(iothread); > + iothread_init_gcontext(iothread, thread_name); > > iothread_set_aio_context_params(base, &local_error); > if (local_error) { I think thread_name might be leaked if error here. Thanks,
On 5/9/23 17:45, Peter Xu wrote: > On Mon, Sep 04, 2023 at 11:48:11AM -0300, Fabiano Rosas wrote: >> @@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp) >> return; >> } >> >> + thread_name = g_strdup_printf("IO %s", >> + object_get_canonical_path_component(OBJECT(base))); >> + >> /* >> * Init one GMainContext for the iothread unconditionally, even if >> * it's not used >> */ >> - iothread_init_gcontext(iothread); >> + iothread_init_gcontext(iothread, thread_name); >> >> iothread_set_aio_context_params(base, &local_error); >> if (local_error) { > > I think thread_name might be leaked if error here. Thanks, Oops, good catch. Better switch to g_autofree.
On Tue, 5 Sept 2023 at 12:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 5/9/23 17:45, Peter Xu wrote: > > On Mon, Sep 04, 2023 at 11:48:11AM -0300, Fabiano Rosas wrote: > >> @@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp) > >> return; > >> } > >> > >> + thread_name = g_strdup_printf("IO %s", > >> + object_get_canonical_path_component(OBJECT(base))); > >> + > >> /* > >> * Init one GMainContext for the iothread unconditionally, even if > >> * it's not used > >> */ > >> - iothread_init_gcontext(iothread); > >> + iothread_init_gcontext(iothread, thread_name); > >> > >> iothread_set_aio_context_params(base, &local_error); > >> if (local_error) { > > > > I think thread_name might be leaked if error here. Thanks, > > Oops, good catch. Better switch to g_autofree. Yes, please. And also make the iothread_init_gcontext(..., char *thread_name) argument const char * to indicate that the name is owned by the caller and not modified by iothread_init_gcontext(). Thanks, Stefan
diff --git a/iothread.c b/iothread.c index b41c305bd9..8cfb9d5be6 100644 --- a/iothread.c +++ b/iothread.c @@ -138,15 +138,19 @@ static void iothread_instance_finalize(Object *obj) qemu_sem_destroy(&iothread->init_done_sem); } -static void iothread_init_gcontext(IOThread *iothread) +static void iothread_init_gcontext(IOThread *iothread, char *thread_name) { GSource *source; + gchar *name = g_strdup_printf("%s aio-context", thread_name); iothread->worker_context = g_main_context_new(); source = aio_get_g_source(iothread_get_aio_context(iothread)); + g_source_set_name(source, name); g_source_attach(source, iothread->worker_context); g_source_unref(source); iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE); + + g_free(name); } static void iothread_set_aio_context_params(EventLoopBase *base, Error **errp) @@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp) return; } + thread_name = g_strdup_printf("IO %s", + object_get_canonical_path_component(OBJECT(base))); + /* * Init one GMainContext for the iothread unconditionally, even if * it's not used */ - iothread_init_gcontext(iothread); + iothread_init_gcontext(iothread, thread_name); iothread_set_aio_context_params(base, &local_error); if (local_error) { @@ -206,8 +213,6 @@ static void iothread_init(EventLoopBase *base, Error **errp) /* This assumes we are called from a thread with useful CPU affinity for us * to inherit. */ - thread_name = g_strdup_printf("IO %s", - object_get_canonical_path_component(OBJECT(base))); qemu_thread_create(&iothread->thread, thread_name, iothread_run, iothread, QEMU_THREAD_JOINABLE); g_free(thread_name);
Having a name in the source helps with debugging core dumps when one might not have access to TLS data to cross-reference AioContexts with their addresses. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- iothread.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)