| Message ID | 20210212172028.288825-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | monitor: Shutdown fixes | expand |
Kevin Wolf <kwolf@redhat.com> writes: > monitor_qmp_dispatcher_co() used to check whether shutdown is requested "used to": until when? > only when it would have to wait for new requests. If there were still > some queued requests, it would try to execute all of them before > shutting down. > > This can be surprising when the queued QMP commands take long or hang > because Ctrl-C may not actually exit QEMU as soon as possible. > > Change monitor_qmp_dispatcher_co() so that it additionally checks > whether shutdown is request before it gets a new request from the queue. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > monitor/qmp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/monitor/qmp.c b/monitor/qmp.c > index 43880fa623..2326bd7f9b 100644 > --- a/monitor/qmp.c > +++ b/monitor/qmp.c > @@ -227,6 +227,11 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data) > */ > qatomic_mb_set(&qmp_dispatcher_co_busy, false); > > + /* On shutdown, don't take any more requests from the queue */ > + if (qmp_dispatcher_co_shutdown) { > + return; > + } > + > while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) { > /* > * No more requests to process. Wait to be reentered from
Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > monitor_qmp_dispatcher_co() used to check whether shutdown is requested > > "used to": until when? Until right before this patch. Do you prefer present tense to describe the old state? Also, does your Tested-by imply that you expect someone else to merge this series? I'm planning to send a pull request today, so if that was your intention, I can include it there. Kevin > > only when it would have to wait for new requests. If there were still > > some queued requests, it would try to execute all of them before > > shutting down. > > > > This can be surprising when the queued QMP commands take long or hang > > because Ctrl-C may not actually exit QEMU as soon as possible. > > > > Change monitor_qmp_dispatcher_co() so that it additionally checks > > whether shutdown is request before it gets a new request from the queue. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > monitor/qmp.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/monitor/qmp.c b/monitor/qmp.c > > index 43880fa623..2326bd7f9b 100644 > > --- a/monitor/qmp.c > > +++ b/monitor/qmp.c > > @@ -227,6 +227,11 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data) > > */ > > qatomic_mb_set(&qmp_dispatcher_co_busy, false); > > > > + /* On shutdown, don't take any more requests from the queue */ > > + if (qmp_dispatcher_co_shutdown) { > > + return; > > + } > > + > > while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) { > > /* > > * No more requests to process. Wait to be reentered from
Kevin Wolf <kwolf@redhat.com> writes: > Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > monitor_qmp_dispatcher_co() used to check whether shutdown is requested >> >> "used to": until when? > > Until right before this patch. Do you prefer present tense to describe > the old state? I've done it both ways. I think present tense works better for me. Case in point: I wasn't confident I read your commit message correctly, so I asked. > Also, does your Tested-by imply that you expect someone else to merge > this series? I'm planning to send a pull request today, so if that was > your intention, I can include it there. I didn't mean to imply anything beyond "I ran my reproducer, and your patches fix it." I don't mind you including the fixes in your pull request. I also don't mind doing a pull request for your fixes. Up to you!
Am 15.02.2021 um 13:42 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > >> > monitor_qmp_dispatcher_co() used to check whether shutdown is requested > >> > >> "used to": until when? > > > > Until right before this patch. Do you prefer present tense to describe > > the old state? > > I've done it both ways. I think present tense works better for me. > Case in point: I wasn't confident I read your commit message correctly, > so I asked. I guess I can just add "Before this patch, ..." to clarify. > > Also, does your Tested-by imply that you expect someone else to merge > > this series? I'm planning to send a pull request today, so if that was > > your intention, I can include it there. > > I didn't mean to imply anything beyond "I ran my reproducer, and your > patches fix it." > > I don't mind you including the fixes in your pull request. I also > don't mind doing a pull request for your fixes. Up to you! Ok, if you don't mind either way, I'll just fix up the commit message and include it. I just asked because it has happened before that everyone was waiting for someone else to merge a patch. So I made it a habit to ask when a maintainer replies with some kind of approval without mentioning that they queued the patch, because that could be a sign that they expect it to go through a different tree. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 15.02.2021 um 13:42 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 15.02.2021 um 13:09 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kwolf@redhat.com> writes: >> >> >> >> > monitor_qmp_dispatcher_co() used to check whether shutdown is requested >> >> >> >> "used to": until when? >> > >> > Until right before this patch. Do you prefer present tense to describe >> > the old state? >> >> I've done it both ways. I think present tense works better for me. >> Case in point: I wasn't confident I read your commit message correctly, >> so I asked. > > I guess I can just add "Before this patch, ..." to clarify. Yes, please. >> > Also, does your Tested-by imply that you expect someone else to merge >> > this series? I'm planning to send a pull request today, so if that was >> > your intention, I can include it there. >> >> I didn't mean to imply anything beyond "I ran my reproducer, and your >> patches fix it." >> >> I don't mind you including the fixes in your pull request. I also >> don't mind doing a pull request for your fixes. Up to you! > > Ok, if you don't mind either way, I'll just fix up the commit message > and include it. > > I just asked because it has happened before that everyone was waiting > for someone else to merge a patch. So I made it a habit to ask when a > maintainer replies with some kind of approval without mentioning that > they queued the patch, because that could be a sign that they expect it > to go through a different tree. Not a bad habit. Thanks!
diff --git a/monitor/qmp.c b/monitor/qmp.c index 43880fa623..2326bd7f9b 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -227,6 +227,11 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data) */ qatomic_mb_set(&qmp_dispatcher_co_busy, false); + /* On shutdown, don't take any more requests from the queue */ + if (qmp_dispatcher_co_shutdown) { + return; + } + while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) { /* * No more requests to process. Wait to be reentered from
monitor_qmp_dispatcher_co() used to check whether shutdown is requested only when it would have to wait for new requests. If there were still some queued requests, it would try to execute all of them before shutting down. This can be surprising when the queued QMP commands take long or hang because Ctrl-C may not actually exit QEMU as soon as possible. Change monitor_qmp_dispatcher_co() so that it additionally checks whether shutdown is request before it gets a new request from the queue. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- monitor/qmp.c | 5 +++++ 1 file changed, 5 insertions(+)