qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts
@ 2018-08-24  9:08 Peter Xu
  2018-08-24  9:08 ` [Qemu-devel] [RFC 1/3] " Peter Xu
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Peter Xu @ 2018-08-24  9:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Marc-André Lureau, Daniel P . Berrange,
	Markus Armbruster, Dr . David Alan Gilbert, peterx

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,

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [RFC 1/3] chardev: introduce chardev contexts
  2018-08-24  9:08 [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Peter Xu
@ 2018-08-24  9:08 ` Peter Xu
  2018-08-24  9:08 ` [Qemu-devel] [RFC 2/3] monitor: generate flag parse helper from init func Peter Xu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2018-08-24  9:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Marc-André Lureau, Daniel P . Berrange,
	Markus Armbruster, Dr . David Alan Gilbert, peterx

Introduce contexts in chardev layer.  Normally chardevs are running with
the main context except the monitors.  Let's create the first standalone
context for the monitors.  Note that this does not mean the chardev
backend will depend on the monitor code, but it's just a naming that let
people know this context should only be used by monitors.

This patch itself should not have any functional change (we'll create a
new iothread that managed by chardev module, but no one is currently
using it), however after this patch we should be able to specify the
context for a chardev by using a qemu_opt_set_number() onto the QemuOps
before the chardev backend is created.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char.c         | 73 +++++++++++++++++++++++++++++++++++++++---
 gdbstub.c              |  2 +-
 hw/bt/hci-csr.c        |  2 +-
 include/chardev/char.h | 15 ++++++++-
 tests/test-char.c      |  4 +--
 5 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 76d866e6fe..c98d245ef7 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -41,6 +41,13 @@
 /***********************************************************/
 /* character device */
 
+typedef struct {
+    IOThread *iothread;
+    GMainContext *gcontext;
+} ChardevContextMap;
+
+static ChardevContextMap *chr_context_table;
+
 static Object *get_chardevs_root(void)
 {
     return container_get(object_get_root(), "/chardevs");
@@ -623,6 +630,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, Error **errp)
     const char *name = chardev_alias_translate(qemu_opt_get(opts, "backend"));
     const char *id = qemu_opts_id(opts);
     char *bid = NULL;
+    ChardevContext context;
 
     if (name && is_help_option(name)) {
         GString *str = g_string_new("");
@@ -634,6 +642,8 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, Error **errp)
         return NULL;
     }
 
+    context = qemu_opt_get_number(opts, "context", CHR_CONTEXT_MAIN);
+
     if (id == NULL) {
         error_setg(errp, "chardev: no id specified");
         return NULL;
@@ -655,7 +665,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, Error **errp)
 
     chr = qemu_chardev_new(bid ? bid : id,
                            object_class_get_name(OBJECT_CLASS(cc)),
-                           backend, errp);
+                           backend, context, errp);
 
     if (chr == NULL) {
         goto out;
@@ -668,7 +678,8 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, Error **errp)
         backend->type = CHARDEV_BACKEND_KIND_MUX;
         backend->u.mux.data = g_new0(ChardevMux, 1);
         backend->u.mux.data->chardev = g_strdup(bid);
-        mux = qemu_chardev_new(id, TYPE_CHARDEV_MUX, backend, errp);
+        mux = qemu_chardev_new(id, TYPE_CHARDEV_MUX,
+                               backend, CHR_CONTEXT_MAIN, errp);
         if (mux == NULL) {
             object_unparent(OBJECT(chr));
             chr = NULL;
@@ -876,6 +887,10 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "logappend",
             .type = QEMU_OPT_BOOL,
+        },{
+            /* TODO: should only be used internally */
+            .name = "context",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end of list */ }
     },
@@ -893,8 +908,42 @@ void qemu_chr_set_feature(Chardev *chr,
     return set_bit(feature, chr->features);
 }
 
+static void qemu_chr_context_init(void)
+{
+    if (!chr_context_table) {
+        ChardevContext i;
+        ChardevContextMap *map;
+
+        chr_context_table = g_new0(ChardevContextMap, CHR_CONTEXT_MAX);
+
+        /* This stands for the main context */
+        chr_context_table[0].iothread = NULL;
+        chr_context_table[0].gcontext = NULL;
+
+        for (i = 1; i < CHR_CONTEXT_MAX; i++) {
+            map = &chr_context_table[i];
+            map->iothread = iothread_create("chr_iothread", &error_abort);
+            map->gcontext = iothread_get_g_main_context(map->iothread);
+        }
+    }
+}
+
+IOThread *qemu_chr_iothread_get(ChardevContext context)
+{
+    assert(context >= 0 && context < CHR_CONTEXT_MAX);
+    qemu_chr_context_init();
+    return chr_context_table[context].iothread;
+}
+
+GMainContext *qemu_chr_context_get(ChardevContext context)
+{
+    assert(context >= 0 && context < CHR_CONTEXT_MAX);
+    qemu_chr_context_init();
+    return chr_context_table[context].gcontext;
+}
+
 Chardev *qemu_chardev_new(const char *id, const char *typename,
-                          ChardevBackend *backend,
+                          ChardevBackend *backend, ChardevContext context,
                           Error **errp)
 {
     Object *obj;
@@ -907,6 +956,7 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
     obj = object_new(typename);
     chr = CHARDEV(obj);
     chr->label = g_strdup(id);
+    chr->gcontext = qemu_chr_context_get(context);
 
     qemu_char_open(chr, backend, &be_opened, &local_err);
     if (local_err) {
@@ -951,7 +1001,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     }
 
     chr = qemu_chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-                           backend, errp);
+                           backend, CHR_CONTEXT_MAIN, errp);
     if (!chr) {
         return NULL;
     }
@@ -1009,7 +1059,7 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
     }
 
     chr_new = qemu_chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
-                               backend, errp);
+                               backend, CHR_CONTEXT_MAIN, errp);
     if (!chr_new) {
         return NULL;
     }
@@ -1102,6 +1152,19 @@ GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms,
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());
+
+    if (chr_context_table) {
+        ChardevContext i;
+        ChardevContextMap *map;
+        for (i = 1; i < CHR_CONTEXT_MAX; i++) {
+            map = &chr_context_table[i];
+            map->gcontext = NULL;
+            iothread_destroy(map->iothread);
+            map->iothread = NULL;
+        }
+        g_free(chr_context_table);
+        chr_context_table = NULL;
+    }
 }
 
 static void register_types(void)
diff --git a/gdbstub.c b/gdbstub.c
index d6ab95006c..777cb257d7 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2052,7 +2052,7 @@ int gdbserver_start(const char *device)
 
         /* Initialize a monitor terminal for gdb */
         mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
-                                   NULL, &error_abort);
+                                   NULL, CHR_CONTEXT_MAIN, &error_abort);
         monitor_init(mon_chr, 0);
     } else {
         qemu_chr_fe_deinit(&s->chr, true);
diff --git a/hw/bt/hci-csr.c b/hw/bt/hci-csr.c
index 0341ded50c..0e36b63301 100644
--- a/hw/bt/hci-csr.c
+++ b/hw/bt/hci-csr.c
@@ -501,7 +501,7 @@ static const TypeInfo char_hci_type_info = {
 Chardev *uart_hci_init(void)
 {
     return qemu_chardev_new(NULL, TYPE_CHARDEV_HCI,
-                            NULL, &error_abort);
+                            NULL, CHR_CONTEXT_MAIN, &error_abort);
 }
 
 static void register_types(void)
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 6f0576e214..e83b93e707 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -5,6 +5,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/bitmap.h"
 #include "qom/object.h"
+#include "sysemu/iothread.h"
 
 #define IAC_EOR 239
 #define IAC_SE 240
@@ -67,6 +68,15 @@ struct Chardev {
     DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
 };
 
+/* This decides the customized context to run the chardev */
+typedef enum {
+    /* Run the chardev in the main context (NULL) */
+    CHR_CONTEXT_MAIN,
+    /* Run the chardev in the monitor specific context */
+    CHR_CONTEXT_MONITOR,
+    CHR_CONTEXT_MAX,
+} ChardevContext;
+
 /**
  * @qemu_chr_new_from_opts:
  *
@@ -262,7 +272,10 @@ typedef struct ChardevClass {
 } ChardevClass;
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
-                          ChardevBackend *backend, Error **errp);
+                          ChardevBackend *backend, ChardevContext context,
+                          Error **errp);
+IOThread *qemu_chr_iothread_get(ChardevContext context);
+GMainContext *qemu_chr_context_get(ChardevContext context);
 
 extern int term_escape_char;
 
diff --git a/tests/test-char.c b/tests/test-char.c
index 5905d31441..ae95e1e3b7 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -593,7 +593,7 @@ static void char_file_fifo_test(void)
     g_assert_cmpint(ret, ==, 8);
 
     chr = qemu_chardev_new("label-file", TYPE_CHARDEV_FILE, &backend,
-                           &error_abort);
+                           CHR_CONTEXT_MAIN, &error_abort);
 
     qemu_chr_fe_init(&be, chr, &error_abort);
     qemu_chr_fe_set_handlers(&be,
@@ -647,7 +647,7 @@ static void char_file_test_internal(Chardev *ext_chr, const char *filepath)
         out = g_build_filename(tmp_path, "out", NULL);
         file.out = out;
         chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
-                               &error_abort);
+                               CHR_CONTEXT_MAIN, &error_abort);
     }
     ret = qemu_chr_write_all(chr, (uint8_t *)"hello!", 6);
     g_assert_cmpint(ret, ==, 6);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [RFC 2/3] monitor: generate flag parse helper from init func
  2018-08-24  9:08 [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Peter Xu
  2018-08-24  9:08 ` [Qemu-devel] [RFC 1/3] " Peter Xu
@ 2018-08-24  9:08 ` Peter Xu
  2018-08-24  9:08 ` [Qemu-devel] [RFC 3/3] monitor: switch to use chardev's iothread Peter Xu
  2018-08-24 13:46 ` [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Marc-André Lureau
  3 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2018-08-24  9:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Marc-André Lureau, Daniel P . Berrange,
	Markus Armbruster, Dr . David Alan Gilbert, peterx

Just to put the flag parsing part into a standalone function, which will
be used in follow up patches.  No functional change.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 vl.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index 16b913f9d5..25e156c020 100644
--- a/vl.c
+++ b/vl.c
@@ -2302,10 +2302,8 @@ static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp)
 }
 #endif
 
-static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
+static int mon_parse_flags(QemuOpts *opts)
 {
-    Chardev *chr;
-    const char *chardev;
     const char *mode;
     int flags;
 
@@ -2330,6 +2328,16 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
         flags |= MONITOR_USE_OOB;
     }
 
+    return flags;
+}
+
+static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
+{
+    Chardev *chr;
+    const char *chardev;
+    int flags;
+
+    flags = mon_parse_flags(opts);
     chardev = qemu_opt_get(opts, "chardev");
     chr = qemu_chr_find(chardev);
     if (chr == NULL) {
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [RFC 3/3] monitor: switch to use chardev's iothread
  2018-08-24  9:08 [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Peter Xu
  2018-08-24  9:08 ` [Qemu-devel] [RFC 1/3] " Peter Xu
  2018-08-24  9:08 ` [Qemu-devel] [RFC 2/3] monitor: generate flag parse helper from init func Peter Xu
@ 2018-08-24  9:08 ` Peter Xu
  2018-08-24 13:46 ` [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Marc-André Lureau
  3 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2018-08-24  9:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Marc-André Lureau, Daniel P . Berrange,
	Markus Armbruster, Dr . David Alan Gilbert, peterx

Now after we have chardev maintained iothread, we switch to that one by
adding a parsing phase for monitors even before chardevs start to
initialize.  Then chardevs will use a constant gcontext since the
creation of the object and it should never change.

Remove the context setup in qemu_chr_be_update_read_handlers(), instead
assert it in qemu_chr_fe_set_handlers() properly to make sure the
context to be setup is always the one provided when chardev creates.

Note that now we kept all the context parameters in misc functions,
however it should be always setting the same context, otherwise a
programming bug.  In the future we may remove all these parameters.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-fe.c |  6 ++++++
 chardev/char.c    |  1 -
 monitor.c         |  4 ++--
 vl.c              | 31 +++++++++++++++++++++++++++++++
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b1f228e8b5..fcb232c9b6 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -267,6 +267,12 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
         remove_fd_in_watch(s);
     } else {
         fe_open = 1;
+        /*
+         * Currently we don't allow to change context after chardev is
+         * created.  TODO: either remove these context parameters, or
+         * re-activate dynamic context switch for chardevs.
+         */
+        assert(context == s->gcontext);
     }
     b->chr_can_read = fd_can_read;
     b->chr_read = fd_read;
diff --git a/chardev/char.c b/chardev/char.c
index c98d245ef7..da8ca9082b 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -200,7 +200,6 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
 {
     ChardevClass *cc = CHARDEV_GET_CLASS(s);
 
-    s->gcontext = context;
     if (cc->chr_update_read_handler) {
         cc->chr_update_read_handler(s);
     }
diff --git a/monitor.c b/monitor.c
index a1999e396c..1e45f3466e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4543,7 +4543,7 @@ static AioContext *monitor_get_aio_context(void)
 
 static void monitor_iothread_init(void)
 {
-    mon_iothread = iothread_create("mon_iothread", &error_abort);
+    mon_iothread = qemu_chr_iothread_get(CHR_CONTEXT_MONITOR);
 
     /*
      * The dispatcher BH must run in the main loop thread, since we
@@ -4735,7 +4735,7 @@ void monitor_cleanup(void)
     qemu_bh_delete(qmp_respond_bh);
     qmp_respond_bh = NULL;
 
-    iothread_destroy(mon_iothread);
+    /* It'll be cleaned up by chardev */
     mon_iothread = NULL;
 }
 
diff --git a/vl.c b/vl.c
index 25e156c020..5bc0946987 100644
--- a/vl.c
+++ b/vl.c
@@ -2349,6 +2349,32 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     return 0;
 }
 
+/*
+ * This setup the backends properly, it really has nothing to do with
+ * monitor itself yet.
+ */
+static int mon_setup_func(void *opaque, QemuOpts *opts, Error **errp)
+{
+    const char *chardev;
+    int flags;
+    QemuOpts *chr_opts;
+
+    flags = mon_parse_flags(opts);
+    chardev = qemu_opt_get(opts, "chardev");
+
+    /*
+     * If out-of-band is enabled on the monitor, choose the correct
+     * context for the backend before it initializes.
+     */
+    if (flags & MONITOR_USE_OOB) {
+        chr_opts = qemu_opts_find(qemu_find_opts("chardev"), chardev);
+        qemu_opt_set_number(chr_opts, "context", CHR_CONTEXT_MONITOR,
+                            &error_abort);
+    }
+
+    return 0;
+}
+
 static void monitor_parse(const char *optarg, const char *mode, bool pretty)
 {
     static int monitor_device_index = 0;
@@ -4278,6 +4304,11 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    if (qemu_opts_foreach(qemu_find_opts("mon"),
+                          mon_setup_func, NULL, NULL)) {
+        exit(1);
+    }
+
     if (qemu_opts_foreach(qemu_find_opts("chardev"),
                           chardev_init_func, NULL, NULL)) {
         exit(1);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts
  2018-08-24  9:08 [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Peter Xu
                   ` (2 preceding siblings ...)
  2018-08-24  9:08 ` [Qemu-devel] [RFC 3/3] monitor: switch to use chardev's iothread Peter Xu
@ 2018-08-24 13:46 ` Marc-André Lureau
  2018-08-27  2:30   ` Peter Xu
  3 siblings, 1 reply; 6+ messages in thread
From: Marc-André Lureau @ 2018-08-24 13:46 UTC (permalink / raw)
  To: Peter Xu; +Cc: QEMU, Markus Armbruster, Dr. David Alan Gilbert, Paolo Bonzini

Hi,

On Fri, Aug 24, 2018 at 11:10 AM Peter Xu <peterx@redhat.com> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts
  2018-08-24 13:46 ` [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Marc-André Lureau
@ 2018-08-27  2:30   ` Peter Xu
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2018-08-27  2:30 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: QEMU, Markus Armbruster, Dr. David Alan Gilbert, Paolo Bonzini

On Fri, Aug 24, 2018 at 03:46:03PM +0200, Marc-André Lureau wrote:
> Hi,
> 
> On Fri, Aug 24, 2018 at 11:10 AM Peter Xu <peterx@redhat.com> 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

Is it because we only have monitor to use it?  If so, I suspect we
might still want it since I'm just thinking maybe we can add a second
one for COLO...

[2]

> - I would not typedef ChardevContext (took me a while to realize it was an enum)

Maybe, ChardevContextType?

> - we should avoid the "context" option, perhaps during chardev
> parsing, check -mon usage.

[2]

> - more :)
> 
> Other issues:
> - blend monitor code in chardev

As I mentioned in the cover letter, my intention is not to blend
monitor code into chardev.  For example, we can define the name as
CHR_CONTEXT_1 rather than CHR_CONTEXT_MONITOR and comment with

  /* CONTEXT_1 should only be used by monitor */

but it's just not that direct.  I'd say it's not "blending monitor
codes in chardev", but a pure naming.  Instead, actually I see [1]
tries to blend monitor code in chardev, which I would prefer not.
That's why I used the QemuOpts to pass and decide what context to use.

> - chardev cleanup is done after monitor cleanup, this may race iothreads

Will it?  My plan is that we only let frontend (monitor) depends on
the frontend (chardev), then cleaning monitor then chardev seems the
correct order for me without race, no?

> - breaks chardev context switching (colo)

I just had a glance at COLO, I'm not sure but... I think it's not
really using the "dynamic context switching".  IMHO it only needs a
separate context.  If so, it should be able to live with this series,
we might just need to define one more for COLO at [1].

> - not a very dynamic solution (doesn't help to create oob monitor dynamically)

I never created monitors dynamically, but if we want that we might
need to expose this "context" property to user, then we can
dynamically create out-of-band monitors (as long as when we create the
chardev for the out-of-band monitor we pass in correct context
property).

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

Thanks for the very positive feedback! :) Yes, let's see whether we
have better alternatives.

Regards,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-08-27  2:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-24  9:08 [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Peter Xu
2018-08-24  9:08 ` [Qemu-devel] [RFC 1/3] " Peter Xu
2018-08-24  9:08 ` [Qemu-devel] [RFC 2/3] monitor: generate flag parse helper from init func Peter Xu
2018-08-24  9:08 ` [Qemu-devel] [RFC 3/3] monitor: switch to use chardev's iothread Peter Xu
2018-08-24 13:46 ` [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts Marc-André Lureau
2018-08-27  2:30   ` Peter Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).