Re: [PATCH] Simple & stupid coroutine-aware monitor_cur()

[Markus Armbruster <armbru@redhat.com>]

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.