Kevin Wolf <kw...@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 <arm...@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.
