Am 04.08.2020 um 15:50 hat Markus Armbruster geschrieben:
> Kevin Wolf <kw...@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 <kw...@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
