Message ID | 87sgcyziul.fsf_-_@dusky.pond.sub.org |
---|---|
State | New |
Headers | show |
Series | Simple & stupid coroutine-aware monitor_cur() | expand |
Am 07.08.2020 um 15:27 hat Markus Armbruster geschrieben: > This is just a sketch. It's incomplete, needs comments and a real > commit message. > > Support for "[PATCH v6 09/12] hmp: Add support for coroutine command > handlers" is missing. Marked FIXME. > > As is, it goes on top of Kevin's series. It is meant to be squashed > into PATCH 06, except for the FIXME, which needs to be resolved in PATCH > 09 instead. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > monitor/monitor.c | 35 +++++++++++++++-------------------- > 1 file changed, 15 insertions(+), 20 deletions(-) > > diff --git a/monitor/monitor.c b/monitor/monitor.c > index 50fb5b20d3..8601340285 100644 > --- a/monitor/monitor.c > +++ b/monitor/monitor.c > @@ -82,38 +82,34 @@ bool qmp_dispatcher_co_shutdown; > */ > bool qmp_dispatcher_co_busy; > > -/* > - * Protects mon_list, monitor_qapi_event_state, coroutine_mon, > - * monitor_destroyed. > - */ > +/* Protects mon_list, monitor_qapi_event_state, * 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 Monitor **monitor_curp(Coroutine *co) > +{ > + static __thread Monitor *thread_local_mon; > + static Monitor *qmp_dispatcher_co_mon; > + > + if (qemu_coroutine_self() == qmp_dispatcher_co) { > + return &qmp_dispatcher_co_mon; > + } > + /* FIXME the coroutine hidden in handle_hmp_command() */ > + return &thread_local_mon; > +} Is thread_local_mon supposed to ever be set? The only callers of monitor_set_cur() are the HMP and QMP dispatchers, which will return something different. So should we return NULL insetad of thread_local_mon... > Monitor *monitor_cur(void) > { > - Monitor *mon; > - > - qemu_mutex_lock(&monitor_lock); > - mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self()); > - qemu_mutex_unlock(&monitor_lock); > - > - return mon; > + return *monitor_curp(qemu_coroutine_self()); > } ...and return NULL here if monitor_curp() returned NULL... > void monitor_set_cur(Coroutine *co, Monitor *mon) > { > - 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); > + *monitor_curp(co) = mon; ...and assert(monitor_curp(co) != NULL) here? This approach looks workable, though the implementation of monitor_curp() feels a bit brittle. The code is not significantly simpler than the hash table based approach, but the assumptions it makes are a bit more hidden. Saving the locks is more a theoretical improvement because all callers are slows paths anyway. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 07.08.2020 um 15:27 hat Markus Armbruster geschrieben: >> This is just a sketch. It's incomplete, needs comments and a real >> commit message. >> >> Support for "[PATCH v6 09/12] hmp: Add support for coroutine command >> handlers" is missing. Marked FIXME. >> >> As is, it goes on top of Kevin's series. It is meant to be squashed >> into PATCH 06, except for the FIXME, which needs to be resolved in PATCH >> 09 instead. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> monitor/monitor.c | 35 +++++++++++++++-------------------- >> 1 file changed, 15 insertions(+), 20 deletions(-) >> >> diff --git a/monitor/monitor.c b/monitor/monitor.c >> index 50fb5b20d3..8601340285 100644 >> --- a/monitor/monitor.c >> +++ b/monitor/monitor.c >> @@ -82,38 +82,34 @@ bool qmp_dispatcher_co_shutdown; >> */ >> bool qmp_dispatcher_co_busy; >> >> -/* >> - * Protects mon_list, monitor_qapi_event_state, coroutine_mon, >> - * monitor_destroyed. >> - */ >> +/* Protects mon_list, monitor_qapi_event_state, * 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 Monitor **monitor_curp(Coroutine *co) >> +{ >> + static __thread Monitor *thread_local_mon; >> + static Monitor *qmp_dispatcher_co_mon; >> + >> + if (qemu_coroutine_self() == qmp_dispatcher_co) { >> + return &qmp_dispatcher_co_mon; >> + } >> + /* FIXME the coroutine hidden in handle_hmp_command() */ >> + return &thread_local_mon; >> +} > > Is thread_local_mon supposed to ever be set? The only callers of > monitor_set_cur() are the HMP and QMP dispatchers, which will return > something different. OOB commands are executed in @mon_iothread, outside coroutine context. qmp_dispatch() calls monitor_set_cur(), which sets thread_local_mon then. Since there is just one @mon_iothread, a @global_mon without __thread would do, but I don't see a need to exploit that here. > So should we return NULL insetad of thread_local_mon... > >> Monitor *monitor_cur(void) >> { >> - Monitor *mon; >> - >> - qemu_mutex_lock(&monitor_lock); >> - mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self()); >> - qemu_mutex_unlock(&monitor_lock); >> - >> - return mon; >> + return *monitor_curp(qemu_coroutine_self()); >> } > > ...and return NULL here if monitor_curp() returned NULL... > >> void monitor_set_cur(Coroutine *co, Monitor *mon) >> { >> - 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); >> + *monitor_curp(co) = mon; > > ...and assert(monitor_curp(co) != NULL) here? > > This approach looks workable, though the implementation of > monitor_curp() feels a bit brittle. The code is not significantly > simpler than the hash table based approach, but the assumptions it makes > are a bit more hidden. > > Saving the locks is more a theoretical improvement because all callers > are slows paths anyway. The hash table only ever has three keys: qmp_dispatcher_co, the coroutine hidden in handle_hmp_command(), and mon_iothread's leader (not in coroutine context). My version replaces the hash table by three pointer variables (two in the sketch above, because I didn't implement the third). You point out my code relies on an argument about which coroutines can execute commands. True. But I have to make that argument anyway to understand how the coroutine-enabled monitor works. On the other hand, it doesn't rely on an argument about the consistency of the hash table with the coroutines.
diff --git a/monitor/monitor.c b/monitor/monitor.c index 50fb5b20d3..8601340285 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -82,38 +82,34 @@ bool qmp_dispatcher_co_shutdown; */ bool qmp_dispatcher_co_busy; -/* - * Protects mon_list, monitor_qapi_event_state, coroutine_mon, - * monitor_destroyed. - */ +/* Protects mon_list, monitor_qapi_event_state, * 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 Monitor **monitor_curp(Coroutine *co) +{ + static __thread Monitor *thread_local_mon; + static Monitor *qmp_dispatcher_co_mon; + + if (qemu_coroutine_self() == qmp_dispatcher_co) { + return &qmp_dispatcher_co_mon; + } + /* FIXME the coroutine hidden in handle_hmp_command() */ + return &thread_local_mon; +} + Monitor *monitor_cur(void) { - Monitor *mon; - - qemu_mutex_lock(&monitor_lock); - mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self()); - qemu_mutex_unlock(&monitor_lock); - - return mon; + return *monitor_curp(qemu_coroutine_self()); } void monitor_set_cur(Coroutine *co, Monitor *mon) { - 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); + *monitor_curp(co) = mon; } /** @@ -666,7 +662,6 @@ 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
This is just a sketch. It's incomplete, needs comments and a real commit message. Support for "[PATCH v6 09/12] hmp: Add support for coroutine command handlers" is missing. Marked FIXME. As is, it goes on top of Kevin's series. It is meant to be squashed into PATCH 06, except for the FIXME, which needs to be resolved in PATCH 09 instead. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- monitor/monitor.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-)