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

From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>,
	marcandre.lureau@gmail.com, qemu-devel@nongnu.org,
	qemu-block@nongnu.org
Subject: Re: [PATCH] Simple & stupid coroutine-aware monitor_cur()
Date: Mon, 10 Aug 2020 14:19:14 +0200	[thread overview]
Message-ID: <20200810121914.GC14538@linux.fritz.box> (raw)
In-Reply-To: <87sgcyziul.fsf_-_@dusky.pond.sub.org>

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