Re: [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts

[Marc-André Lureau]

Hi,

On Fri, Aug 24, 2018 at 11:10 AM Peter Xu <address@hidden> wrote:
>
> This is a RFC series.  It majorly did these things:
>
> (1) move the monitor iothread management from monitor code to chardev
>     code somehow,
>
> (2) decide which context/iothread to use for the chardev before
>     chardev creates, by parsing monitor options earlier (not init, but
>     only parsing) since monitor is the only exception now,
>
> (2) disallow chardev context to change for the whole lifecycle.
>
> Basically this work moves the only chardev iothread (the monitor
> iothread) into chardev's management, then it's easy to setup the
> iothread even before the chardev creates, hence no context switch for
> chardev is needed any more.  In the future if we want to let any
> chardev to run on some other threads, we just define a new
> ChardevContext, then do qemu_opt_set_number(...) for the chardev.  For
> now, there is only a monitor context.
>
> It does not mean that this will let chardev depend on monitor code,
> instead it'll totally remove the reverse dependency - before this
> work, the chardev backend strangely depends on the frontend to setup
> the context (which brought us many trouble).  Now this should be gone.
>
> This series should solve the potential issue raised here:
>
>   https://patchwork.kernel.org/patch/10122713/#22187395
>
> And also should let Marc-André's vhost-user reconnect series work
> without breaking context switch of chardev (since it never switches
> now hence no way to break):
>
>   http://patchwork.ozlabs.org/cover/961442/
>
> Only smoke test carried out with out-of-band, and make check.
>
> Please have a look on whether this is a workable solution,

Clever hack!

The code could be simplified somewhat:
- it probably doesn't need ChardevContextMap
- I would not typedef ChardevContext (took me a while to realize it was an enum)
- we should avoid the "context" option, perhaps during chardev
parsing, check -mon usage.
- more :)

Other issues:
- blend monitor code in chardev
- chardev cleanup is done after monitor cleanup, this may race iothreads
- breaks chardev context switching (colo)
- not a very dynamic solution (doesn't help to create oob monitor dynamically)

I'd continue to look for options, we might come back to this one eventually :)

thanks!

>
> TODO:
> - should not allow user to specify the "context" parameter
> - more ...
>
> Thanks,
>
> Peter Xu (3):
>   chardev: introduce chardev contexts
>   monitor: generate flag parse helper from init func
>   monitor: switch to use chardev's iothread
>
>  chardev/char-fe.c      |  6 ++++
>  chardev/char.c         | 74 ++++++++++++++++++++++++++++++++++++++----
>  gdbstub.c              |  2 +-
>  hw/bt/hci-csr.c        |  2 +-
>  include/chardev/char.h | 15 ++++++++-
>  monitor.c              |  4 +--
>  tests/test-char.c      |  4 +--
>  vl.c                   | 45 +++++++++++++++++++++++--
>  8 files changed, 136 insertions(+), 16 deletions(-)
>
> --
> 2.17.1
>
>


-- 
Marc-André Lureau