| Message ID | 20200115122326.26393-5-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qmp: Optionally run handlers in coroutines | expand |
Kevin Wolf <kwolf@redhat.com> writes: > block_resize is safe to run in a coroutine, so use it as an example for > the new 'coroutine': true annotation in the QAPI schema. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qapi/block-core.json | 3 ++- > blockdev.c | 6 +++--- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7ff5e5edaf..1dbb2a9901 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1341,7 +1341,8 @@ > { 'command': 'block_resize', > 'data': { '*device': 'str', > '*node-name': 'str', > - 'size': 'int' } } > + 'size': 'int' }, > + 'coroutine': true } > > ## > # @NewImageMode: > diff --git a/blockdev.c b/blockdev.c > index 8e029e9c01..b5e5d1e072 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) > aio_context_release(aio_context); > } > > -void qmp_block_resize(bool has_device, const char *device, > - bool has_node_name, const char *node_name, > - int64_t size, Error **errp) > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, > + bool has_node_name, const char *node_name, > + int64_t size, Error **errp) > { > Error *local_err = NULL; > BlockBackend *blk = NULL; Pardon my ignorant question: what exactly makes a function a coroutine_fn?
Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > block_resize is safe to run in a coroutine, so use it as an example for > > the new 'coroutine': true annotation in the QAPI schema. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > diff --git a/blockdev.c b/blockdev.c > > index 8e029e9c01..b5e5d1e072 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) > > aio_context_release(aio_context); > > } > > > > -void qmp_block_resize(bool has_device, const char *device, > > - bool has_node_name, const char *node_name, > > - int64_t size, Error **errp) > > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, > > + bool has_node_name, const char *node_name, > > + int64_t size, Error **errp) > > { > > Error *local_err = NULL; > > BlockBackend *blk = NULL; > > Pardon my ignorant question: what exactly makes a function a > coroutine_fn? When Stefan requested adding the coroutine_fn marker, it seemed to make sense to me because the QMP dispatcher will always call it from coroutine context now, and being always run in coroutine context makes a function a coroutine_fn. However, it's also called from hmp_block_resize(), so at least for now coroutine_fn is actually wrong. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> > block_resize is safe to run in a coroutine, so use it as an example for >> > the new 'coroutine': true annotation in the QAPI schema. >> > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > diff --git a/blockdev.c b/blockdev.c >> > index 8e029e9c01..b5e5d1e072 100644 >> > --- a/blockdev.c >> > +++ b/blockdev.c >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) >> > aio_context_release(aio_context); >> > } >> > >> > -void qmp_block_resize(bool has_device, const char *device, >> > - bool has_node_name, const char *node_name, >> > - int64_t size, Error **errp) >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, >> > + bool has_node_name, const char *node_name, >> > + int64_t size, Error **errp) >> > { >> > Error *local_err = NULL; >> > BlockBackend *blk = NULL; >> >> Pardon my ignorant question: what exactly makes a function a >> coroutine_fn? > > When Stefan requested adding the coroutine_fn marker, it seemed to make > sense to me because the QMP dispatcher will always call it from > coroutine context now, and being always run in coroutine context makes a > function a coroutine_fn. > > However, it's also called from hmp_block_resize(), so at least for now > coroutine_fn is actually wrong. This answers the question when we mark a function a coroutine_fn. I meant to ask what conditions the function itself must satisfy to be eligible for this mark.
Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > block_resize is safe to run in a coroutine, so use it as an example for > >> > the new 'coroutine': true annotation in the QAPI schema. > >> > > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > >> > diff --git a/blockdev.c b/blockdev.c > >> > index 8e029e9c01..b5e5d1e072 100644 > >> > --- a/blockdev.c > >> > +++ b/blockdev.c > >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) > >> > aio_context_release(aio_context); > >> > } > >> > > >> > -void qmp_block_resize(bool has_device, const char *device, > >> > - bool has_node_name, const char *node_name, > >> > - int64_t size, Error **errp) > >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, > >> > + bool has_node_name, const char *node_name, > >> > + int64_t size, Error **errp) > >> > { > >> > Error *local_err = NULL; > >> > BlockBackend *blk = NULL; > >> > >> Pardon my ignorant question: what exactly makes a function a > >> coroutine_fn? > > > > When Stefan requested adding the coroutine_fn marker, it seemed to make > > sense to me because the QMP dispatcher will always call it from > > coroutine context now, and being always run in coroutine context makes a > > function a coroutine_fn. > > > > However, it's also called from hmp_block_resize(), so at least for now > > coroutine_fn is actually wrong. > > This answers the question when we mark a function a coroutine_fn. I > meant to ask what conditions the function itself must satisfy to be > eligible for this mark. The requirement is actually not about the function itself, it's about the callers, as stated above. But being a coroutine_fn allows the function to call other functions that only work in coroutine context (other coroutine_fns). In the end the reason why a function only works in coroutine context is usually that it (or any other coroutine_fns called by it) could yield, which obviously doesn't work outside of coroutine contest. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > block_resize is safe to run in a coroutine, so use it as an example for >> >> > the new 'coroutine': true annotation in the QAPI schema. >> >> > >> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> > >> >> > diff --git a/blockdev.c b/blockdev.c >> >> > index 8e029e9c01..b5e5d1e072 100644 >> >> > --- a/blockdev.c >> >> > +++ b/blockdev.c >> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) >> >> > aio_context_release(aio_context); >> >> > } >> >> > >> >> > -void qmp_block_resize(bool has_device, const char *device, >> >> > - bool has_node_name, const char *node_name, >> >> > - int64_t size, Error **errp) >> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, >> >> > + bool has_node_name, const char *node_name, >> >> > + int64_t size, Error **errp) >> >> > { >> >> > Error *local_err = NULL; >> >> > BlockBackend *blk = NULL; >> >> >> >> Pardon my ignorant question: what exactly makes a function a >> >> coroutine_fn? >> > >> > When Stefan requested adding the coroutine_fn marker, it seemed to make >> > sense to me because the QMP dispatcher will always call it from >> > coroutine context now, and being always run in coroutine context makes a >> > function a coroutine_fn. >> > >> > However, it's also called from hmp_block_resize(), so at least for now >> > coroutine_fn is actually wrong. >> >> This answers the question when we mark a function a coroutine_fn. I >> meant to ask what conditions the function itself must satisfy to be >> eligible for this mark. > > The requirement is actually not about the function itself, it's about > the callers, as stated above. > > But being a coroutine_fn allows the function to call other functions > that only work in coroutine context (other coroutine_fns). In the end > the reason why a function only works in coroutine context is usually > that it (or any other coroutine_fns called by it) could yield, which > obviously doesn't work outside of coroutine contest. Thanks. I think "being always run in coroutine context makes a function a coroutine_fn" is inaccurate. It's "calling a coroutine_fn without switching to coroutine context first when not already in coroutine context". The induction terminates at basic coroutine_fn like qemu_coroutine_yield(). Pertinent: /** * Return whether or not currently inside a coroutine * * This can be used to write functions that work both when in coroutine context * and when not in coroutine context. Note that such functions cannot use the * coroutine_fn annotation since they work outside coroutine context. */ bool qemu_in_coroutine(void); For qmp_block_resize(), it's used like this, in bdrv_truncate(): if (qemu_in_coroutine()) { /* Fast-path if already in coroutine context */ bdrv_truncate_co_entry(&tco); } else { co = qemu_coroutine_create(bdrv_truncate_co_entry, &tco); bdrv_coroutine_enter(child->bs, co); BDRV_POLL_WHILE(child->bs, tco.ret == NOT_DONE); } where bdrv_truncate_co_entry() is a coroutine_fn.
Kevin Wolf <kwolf@redhat.com> writes: > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> > block_resize is safe to run in a coroutine, so use it as an example for >> > the new 'coroutine': true annotation in the QAPI schema. >> > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > diff --git a/blockdev.c b/blockdev.c >> > index 8e029e9c01..b5e5d1e072 100644 >> > --- a/blockdev.c >> > +++ b/blockdev.c >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) >> > aio_context_release(aio_context); >> > } >> > >> > -void qmp_block_resize(bool has_device, const char *device, >> > - bool has_node_name, const char *node_name, >> > - int64_t size, Error **errp) >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, >> > + bool has_node_name, const char *node_name, >> > + int64_t size, Error **errp) >> > { >> > Error *local_err = NULL; >> > BlockBackend *blk = NULL; >> >> Pardon my ignorant question: what exactly makes a function a >> coroutine_fn? > > When Stefan requested adding the coroutine_fn marker, it seemed to make > sense to me because the QMP dispatcher will always call it from > coroutine context now, and being always run in coroutine context makes a > function a coroutine_fn. > > However, it's also called from hmp_block_resize(), so at least for now > coroutine_fn is actually wrong. With the coroutine_fn dropped: Reviewed-by: Markus Armbruster <armbru@redhat.com> Any plans to make more QMP commands 'coroutine': true?
Am 17.01.2020 um 09:13 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > block_resize is safe to run in a coroutine, so use it as an example for > >> > the new 'coroutine': true annotation in the QAPI schema. > >> > > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > >> > diff --git a/blockdev.c b/blockdev.c > >> > index 8e029e9c01..b5e5d1e072 100644 > >> > --- a/blockdev.c > >> > +++ b/blockdev.c > >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) > >> > aio_context_release(aio_context); > >> > } > >> > > >> > -void qmp_block_resize(bool has_device, const char *device, > >> > - bool has_node_name, const char *node_name, > >> > - int64_t size, Error **errp) > >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, > >> > + bool has_node_name, const char *node_name, > >> > + int64_t size, Error **errp) > >> > { > >> > Error *local_err = NULL; > >> > BlockBackend *blk = NULL; > >> > >> Pardon my ignorant question: what exactly makes a function a > >> coroutine_fn? > > > > When Stefan requested adding the coroutine_fn marker, it seemed to make > > sense to me because the QMP dispatcher will always call it from > > coroutine context now, and being always run in coroutine context makes a > > function a coroutine_fn. > > > > However, it's also called from hmp_block_resize(), so at least for now > > coroutine_fn is actually wrong. > > With the coroutine_fn dropped: > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Any plans to make more QMP commands 'coroutine': true? No immediate plans from me. Myself, I was interested in block_resize because without the conversion, Max wanted me to jump through some hoops with a bdrv_truncate() fix to make sure that it wouldn't block the guest during block_resize. Of course, apart from that, there is Marc-André's long-standing screendump bug that will finally have all of the required infrastructure (and I think I saw a patch on the list already). Kevin
Am 17.01.2020 um 06:44 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > >> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: > >> >> Kevin Wolf <kwolf@redhat.com> writes: > >> >> > block_resize is safe to run in a coroutine, so use it as an example for > >> >> > the new 'coroutine': true annotation in the QAPI schema. > >> >> > > >> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > > >> >> > diff --git a/blockdev.c b/blockdev.c > >> >> > index 8e029e9c01..b5e5d1e072 100644 > >> >> > --- a/blockdev.c > >> >> > +++ b/blockdev.c > >> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) > >> >> > aio_context_release(aio_context); > >> >> > } > >> >> > > >> >> > -void qmp_block_resize(bool has_device, const char *device, > >> >> > - bool has_node_name, const char *node_name, > >> >> > - int64_t size, Error **errp) > >> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device, > >> >> > + bool has_node_name, const char *node_name, > >> >> > + int64_t size, Error **errp) > >> >> > { > >> >> > Error *local_err = NULL; > >> >> > BlockBackend *blk = NULL; > >> >> > >> >> Pardon my ignorant question: what exactly makes a function a > >> >> coroutine_fn? > >> > > >> > When Stefan requested adding the coroutine_fn marker, it seemed to make > >> > sense to me because the QMP dispatcher will always call it from > >> > coroutine context now, and being always run in coroutine context makes a > >> > function a coroutine_fn. > >> > > >> > However, it's also called from hmp_block_resize(), so at least for now > >> > coroutine_fn is actually wrong. > >> > >> This answers the question when we mark a function a coroutine_fn. I > >> meant to ask what conditions the function itself must satisfy to be > >> eligible for this mark. > > > > The requirement is actually not about the function itself, it's about > > the callers, as stated above. > > > > But being a coroutine_fn allows the function to call other functions > > that only work in coroutine context (other coroutine_fns). In the end > > the reason why a function only works in coroutine context is usually > > that it (or any other coroutine_fns called by it) could yield, which > > obviously doesn't work outside of coroutine contest. > > Thanks. > > I think "being always run in coroutine context makes a function a > coroutine_fn" is inaccurate. It's "calling a coroutine_fn without > switching to coroutine context first when not already in coroutine > context". The induction terminates at basic coroutine_fn like > qemu_coroutine_yield(). I think we would tend to mark things as coroutine_fn even if they don't call other coroutine_fns (yet), but would be allowed to. But this is now really splitting hairs. Maybe I should just have referred to the documentation in the source: /** * Mark a function that executes in coroutine context * * Functions that execute in coroutine context cannot be called directly from * normal functions. In the future it would be nice to enable compiler or * static checker support for catching such errors. This annotation might make * it possible and in the meantime it serves as documentation. * * For example: * * static void coroutine_fn foo(void) { * .... * } */ #define coroutine_fn Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 17.01.2020 um 06:44 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kwolf@redhat.com> writes: >> >> >> >> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben: [...] >> >> >> Pardon my ignorant question: what exactly makes a function a >> >> >> coroutine_fn? >> >> > >> >> > When Stefan requested adding the coroutine_fn marker, it seemed to make >> >> > sense to me because the QMP dispatcher will always call it from >> >> > coroutine context now, and being always run in coroutine context makes a >> >> > function a coroutine_fn. >> >> > >> >> > However, it's also called from hmp_block_resize(), so at least for now >> >> > coroutine_fn is actually wrong. >> >> >> >> This answers the question when we mark a function a coroutine_fn. I >> >> meant to ask what conditions the function itself must satisfy to be >> >> eligible for this mark. >> > >> > The requirement is actually not about the function itself, it's about >> > the callers, as stated above. >> > >> > But being a coroutine_fn allows the function to call other functions >> > that only work in coroutine context (other coroutine_fns). In the end >> > the reason why a function only works in coroutine context is usually >> > that it (or any other coroutine_fns called by it) could yield, which >> > obviously doesn't work outside of coroutine contest. >> >> Thanks. >> >> I think "being always run in coroutine context makes a function a >> coroutine_fn" is inaccurate. It's "calling a coroutine_fn without >> switching to coroutine context first when not already in coroutine >> context". The induction terminates at basic coroutine_fn like >> qemu_coroutine_yield(). > > I think we would tend to mark things as coroutine_fn even if they don't > call other coroutine_fns (yet), but would be allowed to. But this is now > really splitting hairs. Your hair-splitting is my education :) > Maybe I should just have referred to the documentation in the source: > > /** > * Mark a function that executes in coroutine context > * > * Functions that execute in coroutine context cannot be called directly from > * normal functions. In the future it would be nice to enable compiler or > * static checker support for catching such errors. This annotation might make > * it possible and in the meantime it serves as documentation. > * > * For example: > * > * static void coroutine_fn foo(void) { > * .... > * } > */ > #define coroutine_fn I had read that, of course, but it didn't quite enlighten me, so I asked. Perhaps it would have if it said "Mark a function that expects to run in coroutine context".
diff --git a/qapi/block-core.json b/qapi/block-core.json index 7ff5e5edaf..1dbb2a9901 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1341,7 +1341,8 @@ { 'command': 'block_resize', 'data': { '*device': 'str', '*node-name': 'str', - 'size': 'int' } } + 'size': 'int' }, + 'coroutine': true } ## # @NewImageMode: diff --git a/blockdev.c b/blockdev.c index 8e029e9c01..b5e5d1e072 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) aio_context_release(aio_context); } -void qmp_block_resize(bool has_device, const char *device, - bool has_node_name, const char *node_name, - int64_t size, Error **errp) +void coroutine_fn qmp_block_resize(bool has_device, const char *device, + bool has_node_name, const char *node_name, + int64_t size, Error **errp) { Error *local_err = NULL; BlockBackend *blk = NULL;