Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Kevin Wolf]

Am 04.08.2020 um 15:50 hat Markus Armbruster geschrieben:
> 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().
> >
> > Outside of coroutine context, qemu_coroutine_self() returns the leader
> > coroutine of the current thread.
> 
> Unsaid: you use it as a hash table key to map from coroutine to monitor,
> and for that you need it to return a value unique to the coroutine in
> coroutine context, and a value unique to the thread outside coroutine
> context.  Which qemu_coroutine_self() does.  Correct?

Correct.

> The hash table works, but I hate it just as much as I hate
> pthread_getspecific() / pthread_setspecific().
> 
> What we have here is a need for coroutine-local data.  Feels like a
> perfectly natural concept to me.

If you have a good concept how to implement this in a generic way that
doesn't impact the I/O fast path, feel free to implement it and I'll
happily use it.

But the hash table is simple and works for this use case, so I see
little reason to invest a lot of time in something that we haven't ever
had another user for.

> Are we going to create another hash table whenever we need another piece
> of coroutine-local data?  Or shall we reuse the hash table, suitably
> renamed and moved to another file?

I think I would vote for separate hash tables rather than having a hash
table containing a struct that mixes values from all subsystems, but
this can be discussed when (if) the need arises.

> Why not simply associate an opaque pointer with each coroutine?  All it
> takes is one more member of struct Coroutine.  Whatever creates the
> coroutine decides what to use it for.  The monitor coroutine would use
> it to point to the monitor.

This doesn't work. error_report() is called from all kinds of
coroutines, not just from coroutines created from the monitor, and it
wants to know the current monitor.

> At least, discuss the design alternatives in the commit message.

*sigh* Fine. Tell me which set of alternatives to discuss.

> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/monitor/monitor.h |  2 +-
> >  monitor/hmp.c             |  4 ++--
> >  monitor/monitor.c         | 27 +++++++++++++++++++++------
> >  qapi/qmp-dispatch.c       |  4 ++--
> >  stubs/monitor-core.c      |  2 +-
> >  5 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 43cc746078..16072e325c 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);
> > -void monitor_set_cur(Monitor *mon);
> > +void 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 79be6f26de..3e73a4c3ce 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -1082,9 +1082,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_cur();
> > -    monitor_set_cur(&mon->common);
> > +    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 182ba136b4..35003bb486 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -58,24 +58,38 @@ 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;
> >  }
> >  
> > -void monitor_set_cur(Monitor *mon)
> > +void monitor_set_cur(Coroutine *co, Monitor *mon)
> >  {
> > -    cur_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);
> >  }
> 
> You really need a contract now: any call to monitor_set_cur() with a
> non-null @mon must be followed by a call with a null @mon.

Why? g_hash_table_replace() removes the old value and replaces it with
the new one.

Kevin