Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)

[Markus Armbruster <armbru@redhat.com>]

I called for a discussion of design alternatives, because I dislike the
one I got.  Here we go.

= Context: the "current monitor" =

Output of HMP commands needs to go to the HMP monitor executing the
command.  Trivial in HMP command handlers: the handler function takes a
monitor argument.  Not so trivial in code used both by HMP command
handlers and other users, such as CLI.  In particular, passing the
monitor through multiple layers that don't want to know anything about
monitors to the point that reports an error just so we can make the
error report go where it needs to go would be impractical.  We made
error_report() & friends do the right thing without such help.

To let them do that, we maintain a "current monitor".

    Invariant: while executing a monitor command, thread-local variable
    @cur_mon points to the monitor executing the command.  When the
    thread is not executing a monitor command, @cur_mon is null.

Now error_report() can do the right thing easily: print to @cur_mon if
non-null, else to stderr.

We also use @cur_mon for getting file descriptors stored in the monitor.
Could perhaps do without @cur_mon, but since it's there anyway...

= Problem at hand: "current monitor" for coroutine-enabled commands =

We want to be able to run monitor commands in a coroutine, so they can
yield instead of blocking the main loop.

Simply yielding in a monitor command violates the invariant: we're no
longer executing a monitor command[*], but @cur_mon is still non-null.

This is because the current monitor is no longer a property of the
thread, but a property of the coroutine.  Thread-local variable @cur_mon
doesn't fit the bill anymore.

= Solution 1: A separate map coroutine -> current monitor =

Kevin implemented this, using a hash table.

PRO:

* Stays off the coroutine switch hot path (by staying off coroutine code
  entirely).

CON

* It's a one-off (but at least it's confined to monitor.c)

* It's slow, and uses locks (but that's probably okay for this use; see
  also one-off).

* We get to worry about consistency between coroutines and the hash
  table.

While this looks servicable, I wonder whether we can we come up with
something a bit more elegant.

= Solution 2: Put the map into struct Coroutine =

The hash table can be replaced by putting a @cur_mon member right into
struct Coroutine, together with a setter and a getter function.

PRO

* Stays off the coroutine switch hot path.

CON

* It's a one off.

* HMP bleeds into the coroutine subsystem, which really doesn't want to
  know anything about monitors.

Thanks, but no thanks.

= Solution 3: Put abstract maps into struct Coroutine =

Daniel's proposal: instead of putting a Monitor * member into struct
Coroutine, put an array of void * there, indexed by well-known data
keys.  Initially, there is just one data key, for the current monitor.

This is basically pthread_setspecific(), pthread_getspecific() for
coroutines, with pthread_key_create() dumbed down to a static set of
well-known keys.

PRO

* Stays off the coroutine switch hot path.

* Similar to how thread-local storage works with traditional pthreads.

CON

* Similar to how thread-local storage works with traditional pthreads.

= Solution 4: Fixed coroutine-local storage =

Whereas solution 3 is like traditional pthreads, this solution works
more like __thread does under the hood: we allocate memory for
coroutine-local storage on coroutine creation, maintain a global pointer
on thread switch, and free the memory on destruction.

We can keep the global pointer in struct Coroutine, and have a getter
return it.

If accessing coroutine-local storage ever becomes a performance
bottleneck, we can either open-code the getter, or store the pointer in
thread-local storage (but then we need to update it in the coroutine
switch hot path).  No need to worry about all that now.

Since we don't have compiler and linker support, we have to collect the
coroutine-local variables in a struct manually.

PRO

* Stays off the coroutine switch hot path.

* Access could be made quite fast if need be.

CON

* The struct of coroutine-local variable crosses subsystem boundaries.

= Solution 5: Optional coroutine-specific storage =

When creating a coroutine, you can optionally ask for a certain amount
of coroutine-specific memory.  It's malloced, stored in struct
Coroutine, and freed when on deletion.

A getter returns the coroutine-specific memory.  To actually use it, you
have to know the coroutine's coroutine-specific memory layout.

PRO

* Stays off the coroutine switch hot path.

* Access could be made quite fast if need be.

CON

* Having to know the coroutine's coroutine-specifc memory layout could
  turn out to be impractical for some applications of "property of a
  coroutine".

This is the solution I had in mind from the start.  I have prototype
code that passes basic testing.

= Solution 6: Exploit there is just two coroutines involved =

A simpler solution is possible, but to understand it, you first have to
understand how the threads and coroutines work together.  Let me
recapitulate.

In old QEMU, all monitors run in the main thread's main loop, and
together execute one command after the other.  @cur_mon was a global
variable, to be accessed only by the main thread.

Commit 62aa1d887f "monitor: Fix unsafe sharing of @cur_mon among
threads" (v3.0.0) made @cur_mon thread-local.  "Fix" was a bit of an
overstatement; no unsafe access was known.

The OOB work moved a part of the QMP monitor work from the main loop
into @mon_iothread.  @mon_iothread sends commands to the main thread for
execution, except for commands executed "out-of-band".

This series moves the main thread's QMP command dispatch into coroutine
@qmp_dispatcher_co.  Commands that aren't coroutine-capable get
dispatched to a one-shot bottom half, also in the main thread.

The series modifies the main thread's HMP command dispatch to wrap
execution of each coroutine-capable command in a newly created
coroutine.

We have:

* OOB commands running in @mon_iothread, outside coroutine context

* Coroutine-incapable QMP commands running in the main thread, outside
  coroutine context (detail: in a bottom half)

* Coroutine-incapable HMP commands running in the main thread, outside
  coroutine-incapable context

* Coroutine-capable QMP commands running in the main thread, in
  coroutine @qmp_dispatcher_co

* Coroutine-capable HMP commands runnning in the main thread, in a
  coroutine created just for the command

* At most one non-OOB command is executing at any time.

Let's ignore HMP for now.  Observe:

* As long as there is just one @qmp_dispatcher_co, there is just one
  current monitor for coroutine-capable QMP commands at any time.  It
  can therefore be stored in a simple global variable
  @qmp_dispatcher_co_mon.

* For the coroutine-incapable commands, thread-local variable @cur_mon
  suffices.

* If qemu_coroutine_self() == qmp_dispatcher_co, the current monitor is
  @qmp_dispatcher_co_mon.  Else it's @cur_mon.

To extend this to HMP, we have to make the handle_hmp_command()'s local
variable @co a global one.

PRO:

* Stays off the coroutine switch hot path (by staying off coroutine code
  entirely).

* Simple code.

CON

* It's a one-off (but at least it's confined to monitor.c).

* The argument behind the code is less than simple (see above).

* Should our monitor coroutines multiply, say because we pull off
  executing (some) in-band commands in monitor I/O thread(s), the
  solution falls apart.

I have prototype code that passes basic testing.

Opinions?

I'll post my two prototypes shortly.


[*] In theory, we could yield to a coroutine that is executing another
monitor's monitor command.  In practice, we haven't implemented that.