Re: [PATCH v7 07/13] monitor: Make current monitor a per-coroutine property

[Markus Armbruster]

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;
>  }
>  
[...]