| Message ID | 20200909151149.490589-8-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | monitor: Optionally run handlers in coroutines | expand |
Kevin Wolf <kwolf@redhat.com> writes: > This way, a monitor command handler will still be able to access the > current monitor, but when it yields, all other code code will correctly > get NULL from monitor_cur(). > > This uses a hash table to map the coroutine pointer to the current > monitor of that coroutine. Outside of coroutine context, we associate > the current monitor with the leader coroutine of the current thread. In qemu-system-FOO, the hash table can have only these entries: * (OOB) One mapping @mon_iothread's thread leader to a QMP monitor, while executing a QMP command out-of-band. * (QMP-CO) One mapping @qmp_dispatcher_co (a coroutine in the main thread) to a QMP monitor, while executing a QMP command in-band and in coroutine context. * (QMP) One mapping the main thread's leader to a QMP monitor, while executing a QMP command in-band and out of coroutine context, in a bottom half. * (HMP) One mapping the main thread's leader to an HMP monitor, while executing an HMP command out of coroutine context. * (HMP-CO) One mapping a transient coroutine in the main thread to an HMP monitor, while executing an HMP command in coroutine context. In-band execution is one command after the other. Therefore, at most one monitor command can be executing in-band at any time. Therefore, the hash table has at most *two* entries: one (OOB), and one of the other four. Can you shoot any holes into my argument? I suspect there's a simpler solution struggling to get out. But this solution works, so in it goes. Should the simpler one succeed at getting out, it can go in on top. If not, I'll probably add even more comments to remind myself of these facts. > Approaches to implement some form of coroutine local storage directly in > the coroutine core code have been considered and discarded because they > didn't end up being much more generic than the hash table and their > performance impact on coroutines not using coroutine local storage was > unclear. As the block layer uses a coroutine per I/O request, this is a > fast path and we have to be careful. It's safest to just stay out of > this path with code only used by the monitor. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com>
Am 14.09.2020 um 17:11 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > This way, a monitor command handler will still be able to access the > > current monitor, but when it yields, all other code code will correctly > > get NULL from monitor_cur(). > > > > This uses a hash table to map the coroutine pointer to the current > > monitor of that coroutine. Outside of coroutine context, we associate > > the current monitor with the leader coroutine of the current thread. > > In qemu-system-FOO, the hash table can have only these entries: > > * (OOB) One mapping @mon_iothread's thread leader to a QMP monitor, while > executing a QMP command out-of-band. > > * (QMP-CO) One mapping @qmp_dispatcher_co (a coroutine in the main > thread) to a QMP monitor, while executing a QMP command in-band and in > coroutine context. > > * (QMP) One mapping the main thread's leader to a QMP monitor, while > executing a QMP command in-band and out of coroutine context, in a > bottom half. > > * (HMP) One mapping the main thread's leader to an HMP monitor, while > executing an HMP command out of coroutine context. > > * (HMP-CO) One mapping a transient coroutine in the main thread to an > HMP monitor, while executing an HMP command in coroutine context. > > In-band execution is one command after the other. > > Therefore, at most one monitor command can be executing in-band at any > time. > > Therefore, the hash table has at most *two* entries: one (OOB), and one > of the other four. > > Can you shoot any holes into my argument? I think with human-monitor-command, you can have three mappings: 1. The main thread's leader (it is a non-coroutine QMP command) to the QMP monitor 2. With a coroutine HMP command, one mapping from the transient HMP coroutine to the transient HMP monitor (with a non-coroutine HMP command, we'd instead temporarily change the mapping from 1.) 3. The OOB entry > I suspect there's a simpler solution struggling to get out. But this > solution works, so in it goes. Should the simpler one succeed at > getting out, it can go in on top. If not, I'll probably add even more > comments to remind myself of these facts. I think the simple approach you had posted could work if you can fill in the HMP part, but I think it wasn't completely trivial and you told me not to bother for now, so I didn't. It may still be a viable path forward if you like it better. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 14.09.2020 um 17:11 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > This way, a monitor command handler will still be able to access the >> > current monitor, but when it yields, all other code code will correctly >> > get NULL from monitor_cur(). >> > >> > This uses a hash table to map the coroutine pointer to the current >> > monitor of that coroutine. Outside of coroutine context, we associate >> > the current monitor with the leader coroutine of the current thread. >> >> In qemu-system-FOO, the hash table can have only these entries: >> >> * (OOB) One mapping @mon_iothread's thread leader to a QMP monitor, while >> executing a QMP command out-of-band. >> >> * (QMP-CO) One mapping @qmp_dispatcher_co (a coroutine in the main >> thread) to a QMP monitor, while executing a QMP command in-band and in >> coroutine context. >> >> * (QMP) One mapping the main thread's leader to a QMP monitor, while >> executing a QMP command in-band and out of coroutine context, in a >> bottom half. >> >> * (HMP) One mapping the main thread's leader to an HMP monitor, while >> executing an HMP command out of coroutine context. >> >> * (HMP-CO) One mapping a transient coroutine in the main thread to an >> HMP monitor, while executing an HMP command in coroutine context. >> >> In-band execution is one command after the other. >> >> Therefore, at most one monitor command can be executing in-band at any >> time. >> >> Therefore, the hash table has at most *two* entries: one (OOB), and one >> of the other four. >> >> Can you shoot any holes into my argument? > > I think with human-monitor-command, you can have three mappings: > > 1. The main thread's leader (it is a non-coroutine QMP command) to the > QMP monitor This is (QMP). > 2. With a coroutine HMP command, one mapping from the transient HMP > coroutine to the transient HMP monitor (with a non-coroutine HMP > command, we'd instead temporarily change the mapping from 1.) This is (HMP-CO). > 3. The OOB entry This is (OOB). To get 1. (QMP) and 2, (HMP-CO) at the same time, the in-band, non-coroutine QMP command needs to execute interleaved with the in-band, coroutine HMP command. Such an interleaving contradicts "In-band execution is one command after the other", which is a fundamental assumption in-band commands may make. If the assumption is invalid, we got a problem. Is it? >> I suspect there's a simpler solution struggling to get out. But this >> solution works, so in it goes. Should the simpler one succeed at >> getting out, it can go in on top. If not, I'll probably add even more >> comments to remind myself of these facts. > > I think the simple approach you had posted could work if you can fill in > the HMP part, but I think it wasn't completely trivial and you told me > not to bother for now, Correct. I decided to go with the code you already tested. > so I didn't. It may still be a viable path > forward if you like it better.
Am 28.09.2020 um 09:47 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 14.09.2020 um 17:11 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > >> > This way, a monitor command handler will still be able to access the > >> > current monitor, but when it yields, all other code code will correctly > >> > get NULL from monitor_cur(). > >> > > >> > This uses a hash table to map the coroutine pointer to the current > >> > monitor of that coroutine. Outside of coroutine context, we associate > >> > the current monitor with the leader coroutine of the current thread. > >> > >> In qemu-system-FOO, the hash table can have only these entries: > >> > >> * (OOB) One mapping @mon_iothread's thread leader to a QMP monitor, while > >> executing a QMP command out-of-band. > >> > >> * (QMP-CO) One mapping @qmp_dispatcher_co (a coroutine in the main > >> thread) to a QMP monitor, while executing a QMP command in-band and in > >> coroutine context. > >> > >> * (QMP) One mapping the main thread's leader to a QMP monitor, while > >> executing a QMP command in-band and out of coroutine context, in a > >> bottom half. > >> > >> * (HMP) One mapping the main thread's leader to an HMP monitor, while > >> executing an HMP command out of coroutine context. > >> > >> * (HMP-CO) One mapping a transient coroutine in the main thread to an > >> HMP monitor, while executing an HMP command in coroutine context. > >> > >> In-band execution is one command after the other. > >> > >> Therefore, at most one monitor command can be executing in-band at any > >> time. > >> > >> Therefore, the hash table has at most *two* entries: one (OOB), and one > >> of the other four. > >> > >> Can you shoot any holes into my argument? > > > > I think with human-monitor-command, you can have three mappings: > > > > 1. The main thread's leader (it is a non-coroutine QMP command) to the > > QMP monitor > > This is (QMP). > > > 2. With a coroutine HMP command, one mapping from the transient HMP > > coroutine to the transient HMP monitor (with a non-coroutine HMP > > command, we'd instead temporarily change the mapping from 1.) > > This is (HMP-CO). > > > 3. The OOB entry > > This is (OOB). > > To get 1. (QMP) and 2, (HMP-CO) at the same time, the in-band, > non-coroutine QMP command needs to execute interleaved with the in-band, > coroutine HMP command. > > Such an interleaving contradicts "In-band execution is one command after > the other", which is a fundamental assumption in-band commands may make. > If the assumption is invalid, we got a problem. Is it? Interleaving, or rather executing another command in the middle of its implementation is the very purpose of human-monitor-command (which is what I was talking about, so "the in-band non-coroutine QMP command" is a very specific one). It's the only command I can think of that is exceptional in this way and would lead to three mappings. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 28.09.2020 um 09:47 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 14.09.2020 um 17:11 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kwolf@redhat.com> writes: >> >> >> >> > This way, a monitor command handler will still be able to access the >> >> > current monitor, but when it yields, all other code code will correctly >> >> > get NULL from monitor_cur(). >> >> > >> >> > This uses a hash table to map the coroutine pointer to the current >> >> > monitor of that coroutine. Outside of coroutine context, we associate >> >> > the current monitor with the leader coroutine of the current thread. >> >> >> >> In qemu-system-FOO, the hash table can have only these entries: >> >> >> >> * (OOB) One mapping @mon_iothread's thread leader to a QMP monitor, while >> >> executing a QMP command out-of-band. >> >> >> >> * (QMP-CO) One mapping @qmp_dispatcher_co (a coroutine in the main >> >> thread) to a QMP monitor, while executing a QMP command in-band and in >> >> coroutine context. >> >> >> >> * (QMP) One mapping the main thread's leader to a QMP monitor, while >> >> executing a QMP command in-band and out of coroutine context, in a >> >> bottom half. >> >> >> >> * (HMP) One mapping the main thread's leader to an HMP monitor, while >> >> executing an HMP command out of coroutine context. >> >> >> >> * (HMP-CO) One mapping a transient coroutine in the main thread to an >> >> HMP monitor, while executing an HMP command in coroutine context. >> >> >> >> In-band execution is one command after the other. >> >> >> >> Therefore, at most one monitor command can be executing in-band at any >> >> time. >> >> >> >> Therefore, the hash table has at most *two* entries: one (OOB), and one >> >> of the other four. >> >> >> >> Can you shoot any holes into my argument? >> > >> > I think with human-monitor-command, you can have three mappings: >> > >> > 1. The main thread's leader (it is a non-coroutine QMP command) to the >> > QMP monitor >> >> This is (QMP). >> >> > 2. With a coroutine HMP command, one mapping from the transient HMP >> > coroutine to the transient HMP monitor (with a non-coroutine HMP >> > command, we'd instead temporarily change the mapping from 1.) >> >> This is (HMP-CO). >> >> > 3. The OOB entry >> >> This is (OOB). >> >> To get 1. (QMP) and 2, (HMP-CO) at the same time, the in-band, >> non-coroutine QMP command needs to execute interleaved with the in-band, >> coroutine HMP command. >> >> Such an interleaving contradicts "In-band execution is one command after >> the other", which is a fundamental assumption in-band commands may make. >> If the assumption is invalid, we got a problem. Is it? > > Interleaving, or rather executing another command in the middle of its > implementation is the very purpose of human-monitor-command (which is > what I was talking about, so "the in-band non-coroutine QMP command" is > a very specific one). Got it now, thanks. > It's the only command I can think of that is exceptional in this way > and would lead to three mappings. > > Kevin
Additional nitpick detail on Kevin's request. Kevin Wolf <kwolf@redhat.com> writes: > This way, a monitor command handler will still be able to access the > current monitor, but when it yields, all other code code will correctly > get NULL from monitor_cur(). > > This uses a hash table to map the coroutine pointer to the current > monitor of that coroutine. Outside of coroutine context, we associate > the current monitor with the leader coroutine of the current thread. > > Approaches to implement some form of coroutine local storage directly in > the coroutine core code have been considered and discarded because they > didn't end up being much more generic than the hash table and their > performance impact on coroutines not using coroutine local storage was > unclear. As the block layer uses a coroutine per I/O request, this is a > fast path and we have to be careful. It's safest to just stay out of > this path with code only used by the monitor. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > include/monitor/monitor.h | 2 +- > monitor/hmp.c | 4 ++-- > monitor/monitor.c | 34 +++++++++++++++++++++++++++------- > qapi/qmp-dispatch.c | 4 ++-- > stubs/monitor-core.c | 2 +- > 5 files changed, 33 insertions(+), 13 deletions(-) > > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h > index 7b0ad1de12..026f8a31b2 100644 > --- a/include/monitor/monitor.h > +++ b/include/monitor/monitor.h > @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions; > extern QemuOptsList qemu_mon_opts; > > Monitor *monitor_cur(void); > -Monitor *monitor_set_cur(Monitor *mon); > +Monitor *monitor_set_cur(Coroutine *co, Monitor *mon); > bool monitor_cur_is_qmp(void); > > void monitor_init_globals(void); > diff --git a/monitor/hmp.c b/monitor/hmp.c > index 896c670183..4b66ca1cd6 100644 > --- a/monitor/hmp.c > +++ b/monitor/hmp.c > @@ -1081,9 +1081,9 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline) > } > > /* old_mon is non-NULL when called from qmp_human_monitor_command() */ > - old_mon = monitor_set_cur(&mon->common); > + old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common); > cmd->cmd(&mon->common, qdict); > - monitor_set_cur(old_mon); > + monitor_set_cur(qemu_coroutine_self(), old_mon); > > qobject_unref(qdict); > } > diff --git a/monitor/monitor.c b/monitor/monitor.c > index be3839a7aa..629aa073ee 100644 > --- a/monitor/monitor.c > +++ b/monitor/monitor.c > @@ -58,29 +58,48 @@ IOThread *mon_iothread; > /* Bottom half to dispatch the requests received from I/O thread */ > QEMUBH *qmp_dispatcher_bh; > > -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */ > +/* > + * Protects mon_list, monitor_qapi_event_state, coroutine_mon, > + * monitor_destroyed. > + */ > QemuMutex monitor_lock; > static GHashTable *monitor_qapi_event_state; > +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */ > > MonitorList mon_list; > int mon_refcount; > static bool monitor_destroyed; > > -static __thread Monitor *cur_monitor; > - > Monitor *monitor_cur(void) > { > - return cur_monitor; > + Monitor *mon; > + > + qemu_mutex_lock(&monitor_lock); > + mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self()); > + qemu_mutex_unlock(&monitor_lock); > + > + return mon; > } > > /** > * Sets a new current monitor and returns the old one. > + * > + * If a non-NULL monitor is set for a coroutine, another call resetting it to > + * NULL is required before the coroutine terminates, otherwise a stale entry > + * would remain in the hash table. Wrapping the comment around column 70 or so would make it easier to read. > */ > -Monitor *monitor_set_cur(Monitor *mon) > +Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) > { > - Monitor *old_monitor = cur_monitor; > + Monitor *old_monitor = monitor_cur(); > + > + qemu_mutex_lock(&monitor_lock); > + if (mon) { > + g_hash_table_replace(coroutine_mon, co, mon); > + } else { > + g_hash_table_remove(coroutine_mon, co); > + } > + qemu_mutex_unlock(&monitor_lock); > > - cur_monitor = mon; > return old_monitor; > } > [...]
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 7b0ad1de12..026f8a31b2 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions; extern QemuOptsList qemu_mon_opts; Monitor *monitor_cur(void); -Monitor *monitor_set_cur(Monitor *mon); +Monitor *monitor_set_cur(Coroutine *co, Monitor *mon); bool monitor_cur_is_qmp(void); void monitor_init_globals(void); diff --git a/monitor/hmp.c b/monitor/hmp.c index 896c670183..4b66ca1cd6 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -1081,9 +1081,9 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline) } /* old_mon is non-NULL when called from qmp_human_monitor_command() */ - old_mon = monitor_set_cur(&mon->common); + old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common); cmd->cmd(&mon->common, qdict); - monitor_set_cur(old_mon); + monitor_set_cur(qemu_coroutine_self(), old_mon); qobject_unref(qdict); } diff --git a/monitor/monitor.c b/monitor/monitor.c index be3839a7aa..629aa073ee 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -58,29 +58,48 @@ IOThread *mon_iothread; /* Bottom half to dispatch the requests received from I/O thread */ QEMUBH *qmp_dispatcher_bh; -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */ +/* + * Protects mon_list, monitor_qapi_event_state, coroutine_mon, + * monitor_destroyed. + */ QemuMutex monitor_lock; static GHashTable *monitor_qapi_event_state; +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */ MonitorList mon_list; int mon_refcount; static bool monitor_destroyed; -static __thread Monitor *cur_monitor; - Monitor *monitor_cur(void) { - return cur_monitor; + Monitor *mon; + + qemu_mutex_lock(&monitor_lock); + mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self()); + qemu_mutex_unlock(&monitor_lock); + + return mon; } /** * Sets a new current monitor and returns the old one. + * + * If a non-NULL monitor is set for a coroutine, another call resetting it to + * NULL is required before the coroutine terminates, otherwise a stale entry + * would remain in the hash table. */ -Monitor *monitor_set_cur(Monitor *mon) +Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { - Monitor *old_monitor = cur_monitor; + Monitor *old_monitor = monitor_cur(); + + qemu_mutex_lock(&monitor_lock); + if (mon) { + g_hash_table_replace(coroutine_mon, co, mon); + } else { + g_hash_table_remove(coroutine_mon, co); + } + qemu_mutex_unlock(&monitor_lock); - cur_monitor = mon; return old_monitor; } @@ -623,6 +642,7 @@ void monitor_init_globals_core(void) { monitor_qapi_event_init(); qemu_mutex_init(&monitor_lock); + coroutine_mon = g_hash_table_new(NULL, NULL); /* * The dispatcher BH must run in the main loop thread, since we diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 2fdbc0fba4..5677ba92ca 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -154,11 +154,11 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, } assert(monitor_cur() == NULL); - monitor_set_cur(cur_mon); + monitor_set_cur(qemu_coroutine_self(), cur_mon); cmd->fn(args, &ret, &err); - monitor_set_cur(NULL); + monitor_set_cur(qemu_coroutine_self(), NULL); qobject_unref(args); if (err) { /* or assert(!ret) after reviewing all handlers: */ diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c index dc1748bf13..d058a2a00d 100644 --- a/stubs/monitor-core.c +++ b/stubs/monitor-core.c @@ -8,7 +8,7 @@ Monitor *monitor_cur(void) return NULL; } -Monitor *monitor_set_cur(Monitor *mon) +Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { return NULL; }