| Message ID | 20221005142006.926013-1-afaria@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coroutine: Make qemu_coroutine_self() return NULL if not in coroutine | expand |
Am 05.10.2022 um 16:20 hat Alberto Faria geschrieben: > qemu_coroutine_self() is used in several places outside of coroutine > context (mostly in qcow2 tracing calls). > > Ensure qemu_coroutine_self() works properly when not called from > coroutine context, returning NULL in that case, and remove its > coroutine_fn annotation. > > Signed-off-by: Alberto Faria <afaria@redhat.com> The coroutine_fn annotation for qemu_coroutine_self() is wrong, but I think it already works outside of coroutine context, and consistently in all three backends, by returning &leader. Changing that to NULL makes me kind of nervous because the callers might actually access the leader Coroutine object, and after this change they would crash. (And even if they didn't crash, they wouldn't be able to distinguish the leader coroutines of different threads any more.) Do we have an actual reason to make this chance? That is, do we have any case that was broken before? Kevin
On Wed, Oct 5, 2022 at 5:34 PM Kevin Wolf <kwolf@redhat.com> wrote: > The coroutine_fn annotation for qemu_coroutine_self() is wrong, but I > think it already works outside of coroutine context, and consistently in > all three backends, by returning &leader. > > Changing that to NULL makes me kind of nervous because the callers might > actually access the leader Coroutine object, and after this change they > would crash. (And even if they didn't crash, they wouldn't be able to > distinguish the leader coroutines of different threads any more.) > > Do we have an actual reason to make this chance? That is, do we have any > case that was broken before? No, I just wasn't sure if the current implementations would return a meaningful value when called from outside coroutine context, but it seems they do. I'll send a patch only dropping the coroutine_fn annotation. Thanks, Alberto
On Wed, Oct 05, 2022 at 06:34:09PM +0200, Kevin Wolf wrote: > Am 05.10.2022 um 16:20 hat Alberto Faria geschrieben: > > qemu_coroutine_self() is used in several places outside of coroutine > > context (mostly in qcow2 tracing calls). > > > > Ensure qemu_coroutine_self() works properly when not called from > > coroutine context, returning NULL in that case, and remove its > > coroutine_fn annotation. > > > > Signed-off-by: Alberto Faria <afaria@redhat.com> > > The coroutine_fn annotation for qemu_coroutine_self() is wrong, but I > think it already works outside of coroutine context, and consistently in > all three backends, by returning &leader. Yes, I remember implementing it specifically so that NULL is never returned. The coroutine_fn annotation should be removed. Stefan
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index e55b36f49a..0b9c8b8dac 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -95,9 +95,10 @@ void coroutine_fn qemu_coroutine_yield(void); AioContext *qemu_coroutine_get_aio_context(Coroutine *co); /** - * Get the currently executing coroutine + * Get the currently executing coroutine, or NULL if not called from coroutine + * context */ -Coroutine *coroutine_fn qemu_coroutine_self(void); +Coroutine *qemu_coroutine_self(void); /** * Return whether or not currently inside a coroutine diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c index e2690c5f41..d9d90187a8 100644 --- a/util/coroutine-sigaltstack.c +++ b/util/coroutine-sigaltstack.c @@ -289,9 +289,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_, Coroutine *qemu_coroutine_self(void) { - CoroutineThreadState *s = coroutine_get_thread_state(); + CoroutineThreadState *s = pthread_getspecific(thread_state_key); - return s->current; + return s && s->current->caller ? s->current : NULL; } bool qemu_in_coroutine(void) diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c index ddc98fb4f8..d1dfe0dae5 100644 --- a/util/coroutine-ucontext.c +++ b/util/coroutine-ucontext.c @@ -320,18 +320,18 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_, Coroutine *qemu_coroutine_self(void) { Coroutine *self = get_current(); - CoroutineUContext *leaderp = get_ptr_leader(); - if (!self) { - self = &leaderp->base; - set_current(self); - } + if (self && self->caller) { #ifdef CONFIG_TSAN - if (!leaderp->tsan_co_fiber) { - leaderp->tsan_co_fiber = __tsan_get_current_fiber(); - } + CoroutineUContext *leaderp = get_ptr_leader(); + if (!leaderp->tsan_co_fiber) { + leaderp->tsan_co_fiber = __tsan_get_current_fiber(); + } #endif - return self; + return self; + } else { + return NULL; + } } bool qemu_in_coroutine(void) diff --git a/util/coroutine-win32.c b/util/coroutine-win32.c index 7db2e8f8c8..97a593da7a 100644 --- a/util/coroutine-win32.c +++ b/util/coroutine-win32.c @@ -91,14 +91,7 @@ Coroutine *qemu_coroutine_self(void) { Coroutine *current = get_current(); - if (!current) { - CoroutineWin32 *leader = get_ptr_leader(); - - current = &leader->base; - set_current(current); - leader->fiber = ConvertThreadToFiber(NULL); - } - return current; + return current && current->caller ? current : NULL; } bool qemu_in_coroutine(void)
qemu_coroutine_self() is used in several places outside of coroutine context (mostly in qcow2 tracing calls). Ensure qemu_coroutine_self() works properly when not called from coroutine context, returning NULL in that case, and remove its coroutine_fn annotation. Signed-off-by: Alberto Faria <afaria@redhat.com> --- include/qemu/coroutine.h | 5 +++-- util/coroutine-sigaltstack.c | 4 ++-- util/coroutine-ucontext.c | 18 +++++++++--------- util/coroutine-win32.c | 9 +-------- 4 files changed, 15 insertions(+), 21 deletions(-)