[Qemu-devel] [PATCH 00/14] qio: general non-default GMainContext support

[Qemu-devel] [PATCH 00/14] qio: general non-default GMainContext support

[Peter Xu]

This is another preparation work for monitor OOB seires.

This series tries to allow QIO code to run with non-default
GMainContext.  Note that for most places I kept the old code
untouched, and only modified/introduced new interfaces where there can
be a non-default GMainContext.  The "where" is mostly migration and
chardev submodules, since these two parts of code can be run with
monitor IOThread in the future (which holds a non-default
GMainContext).

These are existing known issue to be solved with GSources that bound
to main thread:

- migration
  - incoming side: still always running on main context, while we need
    to be able to run some command in OOB thread [1]
- tcp chardev (non-tcp chardevs should all support non-NULL context now)
  - server listening mode: QIO net listener used [2]
  - TELNET session: an isolated GSource used (tcp_chr_telnet_init) [3]
  - when "reconnect=N" is used, QIO threaded task is used [4]
  - TLS session: QIO tls handshake is used (tcp_chr_tls_init) [5]

Patch 1-2 are cleanups and fixes.

Patch 3 introduced qio_channel_add_watch_full(), which is the core API
for QIO to support non-default context.

Patch 4 fixes the migration usage of QIO, which is problem [1] above.

Patch 5-7 fixes the net listeners to use non-default gcontext, which
solves problem [2] above.

Patch 8 fixes the TELNET GSource, which solves problem [3].

Patch 9-13 fixes the threaded QIOTask usage, which is for problem [4].

Patch 14 fixes the last TLS usage, which is problem [5].

The whole series survives with "make check".  There are quite a few
QIO tests there.  Let's see whether this can be acceptable before more
tests.

Please review.  Thanks.

Peter Xu (14):
  chardev: fix leak in tcp_chr_telnet_init_io()
  qio: rename qio_task_thread_result
  qio: introduce qio_channel_add_watch_full()
  migration: let incoming side use thread context
  qio: refactor net listener source operations
  qio: store gsources for net listeners
  qio/chardev: update net listener gcontext
  chardev: allow telnet gsource to switch gcontext
  qio: basic non-default context support for thread
  qio: refcount QIOTask
  qio/chardev: return QIOTask when connect async
  qio: move QIOTaskThreadData into QIOTask
  qio: allow threaded qiotask to switch contexts
  qio/chardev: specify gcontext for TLS handshake

 chardev/char-socket.c       | 114 ++++++++++++++++++++++++++-------
 include/io/channel-socket.h |  14 +++--
 include/io/channel-tls.h    |  22 ++++++-
 include/io/channel.h        |  31 ++++++++-
 include/io/net-listener.h   |  33 +++++++++-
 include/io/task.h           |  10 ++-
 io/channel-socket.c         |  21 ++++---
 io/channel-tls.c            |  91 ++++++++++++++++++++++-----
 io/channel.c                |  24 +++++--
 io/dns-resolver.c           |   3 +-
 io/net-listener.c           | 119 +++++++++++++++++++++--------------
 io/task.c                   | 149 ++++++++++++++++++++++++++++++++++++--------
 migration/exec.c            |  11 ++--
 migration/fd.c              |  11 ++--
 migration/socket.c          |  12 ++--
 tests/test-io-task.c        |   2 +
 16 files changed, 514 insertions(+), 153 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 01/14] chardev: fix leak in tcp_chr_telnet_init_io()

[Peter Xu]

Need to free TCPChardevTelnetInit when session established.

Since at it, switch to use G_SOURCE_* macros.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index bdd6cff5f6..43a2cc2c1c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -591,19 +591,23 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
             ret = 0;
         } else {
             tcp_chr_disconnect(init->chr);
-            return FALSE;
+            goto end;
         }
     }
     init->buflen -= ret;
 
     if (init->buflen == 0) {
         tcp_chr_connect(init->chr);
-        return FALSE;
+        goto end;
     }
 
     memmove(init->buf, init->buf + ret, init->buflen);
 
-    return TRUE;
+    return G_SOURCE_CONTINUE;
+
+end:
+    g_free(init);
+    return G_SOURCE_REMOVE;
 }
 
 static void tcp_chr_telnet_init(Chardev *chr)
-- 
2.14.3

[Qemu-devel] [PATCH 02/14] qio: rename qio_task_thread_result

[Peter Xu]

It is strange that it was called gio_task_thread_result.  Rename it to
follow the naming rule of the file.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 io/task.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/task.c b/io/task.c
index 3ce556017c..1a0a1c7185 100644
--- a/io/task.c
+++ b/io/task.c
@@ -80,7 +80,7 @@ struct QIOTaskThreadData {
 };
 
 
-static gboolean gio_task_thread_result(gpointer opaque)
+static gboolean qio_task_thread_result(gpointer opaque)
 {
     struct QIOTaskThreadData *data = opaque;
 
@@ -110,7 +110,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
      * the worker results
      */
     trace_qio_task_thread_exit(data->task);
-    g_idle_add(gio_task_thread_result, data);
+    g_idle_add(qio_task_thread_result, data);
     return NULL;
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()

[Peter Xu]

It's a more powerful version of qio_channel_add_watch(), which supports
non-default gcontext.  It's stripped from the old one, then we have
g_source_get_id() to fetch the tag ID to keep the old interface.

Note that the new API will return a gsource, meanwhile keep a reference
of it so that callers need to unref them explicitly.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/io/channel.h | 31 ++++++++++++++++++++++++++++---
 io/channel.c         | 24 +++++++++++++++++++-----
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 3995e243a3..36af5e58ae 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -620,20 +620,45 @@ GSource *qio_channel_create_watch(QIOChannel *ioc,
                                   GIOCondition condition);
 
 /**
- * qio_channel_add_watch:
+ * qio_channel_add_watch_full:
  * @ioc: the channel object
  * @condition: the I/O condition to monitor
  * @func: callback to invoke when the source becomes ready
  * @user_data: opaque data to pass to @func
  * @notify: callback to free @user_data
+ * @context: gcontext to bind the source to
  *
- * Create a new main loop source that is used to watch
+ * Create a new source that is used to watch
  * for the I/O condition @condition. The callback @func
  * will be registered against the source, to be invoked
  * when the source becomes ready. The optional @user_data
  * will be passed to @func when it is invoked. The @notify
  * callback will be used to free @user_data when the
- * watch is deleted
+ * watch is deleted.  The source will be bound to @context if
+ * provided, or main context if it is NULL.
+ *
+ * Note: if a valid source is returned, we need to explicitly unref
+ * the source to destroy it.
+ *
+ * Returns: the source pointer
+ */
+GSource *qio_channel_add_watch_full(QIOChannel *ioc,
+                                    GIOCondition condition,
+                                    QIOChannelFunc func,
+                                    gpointer user_data,
+                                    GDestroyNotify notify,
+                                    GMainContext *context);
+
+/**
+ * qio_channel_add_watch:
+ * @ioc: the channel object
+ * @condition: the I/O condition to monitor
+ * @func: callback to invoke when the source becomes ready
+ * @user_data: opaque data to pass to @func
+ * @notify: callback to free @user_data
+ *
+ * Wrapper of qio_channel_add_watch_full(), but it'll only bind the
+ * source object to default main context.
  *
  * The returned source ID can be used with g_source_remove()
  * to remove and free the source when no longer required.
diff --git a/io/channel.c b/io/channel.c
index ec4b86de7c..3e734cc9a5 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -299,6 +299,22 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc,
     klass->io_set_aio_fd_handler(ioc, ctx, io_read, io_write, opaque);
 }
 
+GSource *qio_channel_add_watch_full(QIOChannel *ioc,
+                                    GIOCondition condition,
+                                    QIOChannelFunc func,
+                                    gpointer user_data,
+                                    GDestroyNotify notify,
+                                    GMainContext *context)
+{
+    GSource *source;
+
+    source = qio_channel_create_watch(ioc, condition);
+    g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
+    g_source_attach(source, context);
+
+    return source;
+}
+
 guint qio_channel_add_watch(QIOChannel *ioc,
                             GIOCondition condition,
                             QIOChannelFunc func,
@@ -308,11 +324,9 @@ guint qio_channel_add_watch(QIOChannel *ioc,
     GSource *source;
     guint id;
 
-    source = qio_channel_create_watch(ioc, condition);
-
-    g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
-
-    id = g_source_attach(source, NULL);
+    source = qio_channel_add_watch_full(ioc, condition, func,
+                                        user_data, notify, NULL);
+    id = g_source_get_id(source);
     g_source_unref(source);
 
     return id;
-- 
2.14.3

[Qemu-devel] [PATCH 04/14] migration: let incoming side use thread context

[Peter Xu]

The old incoming migration is running in main thread and default
gcontext.  With the new qio_channel_add_watch_full() we can now let it
run in the thread's own gcontext (if there is one).

Currently this patch does nothing alone.  But when any of the incoming
migration is run in another iothread (e.g., the upcoming migrate-recover
command), this patch will bind the incoming logic to the iothread
instead of the main thread (which may already get page faulted and
hanged).

RDMA is not considered for now since it's not even using the QIO APIs at
all.

CC: Juan Quintela <quintela@redhat.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/exec.c   | 11 ++++++-----
 migration/fd.c     | 11 ++++++-----
 migration/socket.c | 12 +++++++-----
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 0bc5a427dd..f401fc005e 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -55,6 +55,7 @@ void exec_start_incoming_migration(const char *command, Error **errp)
 {
     QIOChannel *ioc;
     const char *argv[] = { "/bin/sh", "-c", command, NULL };
+    GSource *source;
 
     trace_migration_exec_incoming(command);
     ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
@@ -65,9 +66,9 @@ void exec_start_incoming_migration(const char *command, Error **errp)
     }
 
     qio_channel_set_name(ioc, "migration-exec-incoming");
-    qio_channel_add_watch(ioc,
-                          G_IO_IN,
-                          exec_accept_incoming_migration,
-                          NULL,
-                          NULL);
+    source = qio_channel_add_watch_full(ioc, G_IO_IN,
+                                        exec_accept_incoming_migration,
+                                        NULL, NULL,
+                                        g_main_context_get_thread_default());
+    g_source_unref(source);
 }
diff --git a/migration/fd.c b/migration/fd.c
index cd06182d1e..9c593eb3ff 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -55,6 +55,7 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
 {
     QIOChannel *ioc;
     int fd;
+    GSource *source;
 
     fd = strtol(infd, NULL, 0);
     trace_migration_fd_incoming(fd);
@@ -66,9 +67,9 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
     }
 
     qio_channel_set_name(QIO_CHANNEL(ioc), "migration-fd-incoming");
-    qio_channel_add_watch(ioc,
-                          G_IO_IN,
-                          fd_accept_incoming_migration,
-                          NULL,
-                          NULL);
+    source = qio_channel_add_watch_full(ioc, G_IO_IN,
+                                        fd_accept_incoming_migration,
+                                        NULL, NULL,
+                                        g_main_context_get_thread_default());
+    g_source_unref(source);
 }
diff --git a/migration/socket.c b/migration/socket.c
index e090097077..82c330083c 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -164,6 +164,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
                                             Error **errp)
 {
     QIOChannelSocket *listen_ioc = qio_channel_socket_new();
+    GSource *source;
 
     qio_channel_set_name(QIO_CHANNEL(listen_ioc),
                          "migration-socket-listener");
@@ -173,11 +174,12 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
         return;
     }
 
-    qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
-                          G_IO_IN,
-                          socket_accept_incoming_migration,
-                          listen_ioc,
-                          (GDestroyNotify)object_unref);
+    source = qio_channel_add_watch_full(QIO_CHANNEL(listen_ioc), G_IO_IN,
+                                        socket_accept_incoming_migration,
+                                        listen_ioc,
+                                        (GDestroyNotify)object_unref,
+                                        g_main_context_get_thread_default());
+    g_source_unref(source);
 }
 
 void tcp_start_incoming_migration(const char *host_port, Error **errp)
-- 
2.14.3

[Qemu-devel] [PATCH 05/14] qio: refactor net listener source operations

[Peter Xu]

Three functions are abstracted from the old code:

- qio_net_listener_source_add(): create one source for listener
- qio_net_listener_sources_clear(): unset existing net lister sources
- qio_net_listener_sources_update(): setup all sources for listener

Use them where possible.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 io/net-listener.c | 82 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/io/net-listener.c b/io/net-listener.c
index de38dfae99..3e9ac51b0e 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -106,6 +106,39 @@ int qio_net_listener_open_sync(QIONetListener *listener,
     }
 }
 
+static guint qio_net_listener_source_add(QIONetListener *listener,
+                                         QIOChannelSocket *sioc)
+{
+    return qio_channel_add_watch(QIO_CHANNEL(sioc), G_IO_IN,
+                                 qio_net_listener_channel_func,
+                                 listener, (GDestroyNotify)object_unref);
+}
+
+static void qio_net_listener_sources_clear(QIONetListener *listener)
+{
+    size_t i;
+
+    for (i = 0; i < listener->nsioc; i++) {
+        if (listener->io_tag[i]) {
+            g_source_remove(listener->io_tag[i]);
+            listener->io_tag[i] = 0;
+        }
+    }
+}
+
+static void qio_net_listener_sources_update(QIONetListener *listener)
+{
+    size_t i;
+
+    if (listener->io_func != NULL) {
+        for (i = 0; i < listener->nsioc; i++) {
+            assert(listener->io_tag[i] == 0);
+            object_ref(OBJECT(listener));
+            listener->io_tag[i] = qio_net_listener_source_add(
+                listener, listener->sioc[i]);
+        }
+    }
+}
 
 void qio_net_listener_add(QIONetListener *listener,
                           QIOChannelSocket *sioc)
@@ -127,10 +160,8 @@ void qio_net_listener_add(QIONetListener *listener,
 
     if (listener->io_func != NULL) {
         object_ref(OBJECT(listener));
-        listener->io_tag[listener->nsioc] = qio_channel_add_watch(
-            QIO_CHANNEL(listener->sioc[listener->nsioc]), G_IO_IN,
-            qio_net_listener_channel_func,
-            listener, (GDestroyNotify)object_unref);
+        listener->io_tag[listener->nsioc] = qio_net_listener_source_add(
+            listener, listener->sioc[listener->nsioc]);
     }
 
     listener->nsioc++;
@@ -142,8 +173,6 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
                                       gpointer data,
                                       GDestroyNotify notify)
 {
-    size_t i;
-
     if (listener->io_notify) {
         listener->io_notify(listener->io_data);
     }
@@ -151,22 +180,8 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
     listener->io_data = data;
     listener->io_notify = notify;
 
-    for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
-        }
-    }
-
-    if (listener->io_func != NULL) {
-        for (i = 0; i < listener->nsioc; i++) {
-            object_ref(OBJECT(listener));
-            listener->io_tag[i] = qio_channel_add_watch(
-                QIO_CHANNEL(listener->sioc[i]), G_IO_IN,
-                qio_net_listener_channel_func,
-                listener, (GDestroyNotify)object_unref);
-        }
-    }
+    qio_net_listener_sources_clear(listener);
+    qio_net_listener_sources_update(listener);
 }
 
 
@@ -210,12 +225,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
     };
     size_t i;
 
-    for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
-        }
-    }
+    qio_net_listener_sources_clear(listener);
 
     sources = g_new0(GSource *, listener->nsioc);
     for (i = 0; i < listener->nsioc; i++) {
@@ -238,15 +248,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
     g_main_loop_unref(loop);
     g_main_context_unref(ctxt);
 
-    if (listener->io_func != NULL) {
-        for (i = 0; i < listener->nsioc; i++) {
-            object_ref(OBJECT(listener));
-            listener->io_tag[i] = qio_channel_add_watch(
-                QIO_CHANNEL(listener->sioc[i]), G_IO_IN,
-                qio_net_listener_channel_func,
-                listener, (GDestroyNotify)object_unref);
-        }
-    }
+    qio_net_listener_sources_update(listener);
 
     return data.sioc;
 }
@@ -259,11 +261,9 @@ void qio_net_listener_disconnect(QIONetListener *listener)
         return;
     }
 
+    qio_net_listener_sources_clear(listener);
+
     for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
-        }
         qio_channel_close(QIO_CHANNEL(listener->sioc[i]), NULL);
     }
     listener->connected = false;
-- 
2.14.3

[Qemu-devel] [PATCH 06/14] qio: store gsources for net listeners

[Peter Xu]

Originally we were storing the GSources tag IDs.  That'll be not enough
if we are going to support non-default gcontext for QIO code.  Switch to
GSources without changing anything real.  Now we still always pass in
NULL, which means the default gcontext.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/io/net-listener.h | 21 ++++++++++++++--
 io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/include/io/net-listener.h b/include/io/net-listener.h
index 56d6da7a76..566be283b3 100644
--- a/include/io/net-listener.h
+++ b/include/io/net-listener.h
@@ -53,7 +53,7 @@ struct QIONetListener {
 
     char *name;
     QIOChannelSocket **sioc;
-    gulong *io_tag;
+    GSource **io_source;
     size_t nsioc;
 
     bool connected;
@@ -120,17 +120,34 @@ void qio_net_listener_add(QIONetListener *listener,
                           QIOChannelSocket *sioc);
 
 /**
- * qio_net_listener_set_client_func:
+ * qio_net_listener_set_client_func_full:
  * @listener: the network listener object
  * @func: the callback function
  * @data: opaque data to pass to @func
  * @notify: callback to free @data
+ * @context: the context that the sources will be bound to
  *
  * Register @func to be invoked whenever a new client
  * connects to the listener. @func will be invoked
  * passing in the QIOChannelSocket instance for the
  * client.
  */
+void qio_net_listener_set_client_func_full(QIONetListener *listener,
+                                           QIONetListenerClientFunc func,
+                                           gpointer data,
+                                           GDestroyNotify notify,
+                                           GMainContext *context);
+
+/**
+ * qio_net_listener_set_client_func:
+ * @listener: the network listener object
+ * @func: the callback function
+ * @data: opaque data to pass to @func
+ * @notify: callback to free @data
+ *
+ * Wrapper of qio_net_listener_set_client_func_full(), only that the
+ * sources will always be bound to default main context.
+ */
 void qio_net_listener_set_client_func(QIONetListener *listener,
                                       QIONetListenerClientFunc func,
                                       gpointer data,
diff --git a/io/net-listener.c b/io/net-listener.c
index 3e9ac51b0e..7f07a81fed 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -106,12 +106,15 @@ int qio_net_listener_open_sync(QIONetListener *listener,
     }
 }
 
-static guint qio_net_listener_source_add(QIONetListener *listener,
-                                         QIOChannelSocket *sioc)
+static GSource *qio_net_listener_source_add(QIONetListener *listener,
+                                            QIOChannelSocket *sioc,
+                                            GMainContext *context)
 {
-    return qio_channel_add_watch(QIO_CHANNEL(sioc), G_IO_IN,
-                                 qio_net_listener_channel_func,
-                                 listener, (GDestroyNotify)object_unref);
+    return qio_channel_add_watch_full(QIO_CHANNEL(sioc), G_IO_IN,
+                                      qio_net_listener_channel_func,
+                                      listener,
+                                      (GDestroyNotify)object_unref,
+                                      context);
 }
 
 static void qio_net_listener_sources_clear(QIONetListener *listener)
@@ -119,23 +122,25 @@ static void qio_net_listener_sources_clear(QIONetListener *listener)
     size_t i;
 
     for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
+        if (listener->io_source[i]) {
+            g_source_destroy(listener->io_source[i]);
+            g_source_unref(listener->io_source[i]);
+            listener->io_source[i] = NULL;
         }
     }
 }
 
-static void qio_net_listener_sources_update(QIONetListener *listener)
+static void qio_net_listener_sources_update(QIONetListener *listener,
+                                            GMainContext *context)
 {
     size_t i;
 
     if (listener->io_func != NULL) {
         for (i = 0; i < listener->nsioc; i++) {
-            assert(listener->io_tag[i] == 0);
+            assert(listener->io_source[i] == NULL);
             object_ref(OBJECT(listener));
-            listener->io_tag[i] = qio_net_listener_source_add(
-                listener, listener->sioc[i]);
+            listener->io_source[i] = qio_net_listener_source_add(
+                listener, listener->sioc[i], context);
         }
     }
 }
@@ -151,27 +156,30 @@ void qio_net_listener_add(QIONetListener *listener,
 
     listener->sioc = g_renew(QIOChannelSocket *, listener->sioc,
                              listener->nsioc + 1);
-    listener->io_tag = g_renew(gulong, listener->io_tag, listener->nsioc + 1);
+    listener->io_source = g_renew(typeof(listener->io_source[0]),
+                                  listener->io_source,
+                                  listener->nsioc + 1);
     listener->sioc[listener->nsioc] = sioc;
-    listener->io_tag[listener->nsioc] = 0;
+    listener->io_source[listener->nsioc] = NULL;
 
     object_ref(OBJECT(sioc));
     listener->connected = true;
 
     if (listener->io_func != NULL) {
         object_ref(OBJECT(listener));
-        listener->io_tag[listener->nsioc] = qio_net_listener_source_add(
-            listener, listener->sioc[listener->nsioc]);
+        listener->io_source[listener->nsioc] = qio_net_listener_source_add(
+            listener, listener->sioc[listener->nsioc], NULL);
     }
 
     listener->nsioc++;
 }
 
 
-void qio_net_listener_set_client_func(QIONetListener *listener,
-                                      QIONetListenerClientFunc func,
-                                      gpointer data,
-                                      GDestroyNotify notify)
+void qio_net_listener_set_client_func_full(QIONetListener *listener,
+                                           QIONetListenerClientFunc func,
+                                           gpointer data,
+                                           GDestroyNotify notify,
+                                           GMainContext *context)
 {
     if (listener->io_notify) {
         listener->io_notify(listener->io_data);
@@ -181,9 +189,17 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
     listener->io_notify = notify;
 
     qio_net_listener_sources_clear(listener);
-    qio_net_listener_sources_update(listener);
+    qio_net_listener_sources_update(listener, context);
 }
 
+void qio_net_listener_set_client_func(QIONetListener *listener,
+                                      QIONetListenerClientFunc func,
+                                      gpointer data,
+                                      GDestroyNotify notify)
+{
+    qio_net_listener_set_client_func_full(listener, func, data,
+                                          notify, NULL);
+}
 
 struct QIONetListenerClientWaitData {
     QIOChannelSocket *sioc;
@@ -248,7 +264,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
     g_main_loop_unref(loop);
     g_main_context_unref(ctxt);
 
-    qio_net_listener_sources_update(listener);
+    qio_net_listener_sources_update(listener, NULL);
 
     return data.sioc;
 }
@@ -285,7 +301,7 @@ static void qio_net_listener_finalize(Object *obj)
     for (i = 0; i < listener->nsioc; i++) {
         object_unref(OBJECT(listener->sioc[i]));
     }
-    g_free(listener->io_tag);
+    g_free(listener->io_source);
     g_free(listener->sioc);
     g_free(listener->name);
 }
-- 
2.14.3

[Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext

[Peter Xu]

TCP chardevs can be using QIO network listeners working in the
background when in listening mode.  However the network listeners are
always running in main context.  This can race with chardevs that are
running in non-main contexts.

To solve this: firstly introduce qio_net_listener_set_context() to allow
caller to set gcontext for network listeners.  Then call it in
tcp_chr_update_read_handler(), with the newly cached gcontext.

It's fairly straightforward after we have introduced some net listener
helper functions - basically we unregister the GSources and add them
back with the correct context.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c     |  9 +++++++++
 include/io/net-listener.h | 12 ++++++++++++
 io/net-listener.c         |  7 +++++++
 3 files changed, 28 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 43a2cc2c1c..8f0935cd15 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
+    if (s->listener) {
+        /*
+         * It's possible that chardev context is changed in
+         * qemu_chr_be_update_read_handlers().  Reset it for QIO net
+         * listener if there is.
+         */
+        qio_net_listener_set_context(s->listener, chr->gcontext);
+    }
+
     if (!s->connected) {
         return;
     }
diff --git a/include/io/net-listener.h b/include/io/net-listener.h
index 566be283b3..39dede9d6f 100644
--- a/include/io/net-listener.h
+++ b/include/io/net-listener.h
@@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
                                SocketAddress *addr,
                                Error **errp);
 
+/**
+ * qio_net_listener_set_context:
+ * @listener: the net listener object
+ * @context: the context that we'd like to bind the sources to
+ *
+ * This helper does not do anything but moves existing net listener
+ * sources from the old one to the new one.  It can be seen as a
+ * no-operation if there is no listening source at all.
+ */
+void qio_net_listener_set_context(QIONetListener *listener,
+                                  GMainContext *context);
+
 /**
  * qio_net_listener_add:
  * @listener: the network listener object
diff --git a/io/net-listener.c b/io/net-listener.c
index 7f07a81fed..7ffad72f55 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -145,6 +145,13 @@ static void qio_net_listener_sources_update(QIONetListener *listener,
     }
 }
 
+void qio_net_listener_set_context(QIONetListener *listener,
+                                  GMainContext *context)
+{
+    qio_net_listener_sources_clear(listener);
+    qio_net_listener_sources_update(listener, context);
+}
+
 void qio_net_listener_add(QIONetListener *listener,
                           QIOChannelSocket *sioc)
 {
-- 
2.14.3

[Qemu-devel] [PATCH 08/14] chardev: allow telnet gsource to switch gcontext

[Peter Xu]

It was originally created by qio_channel_add_watch() so it's always
assigning the task to main context.  Now we use the new API called
qio_channel_add_watch_full() so that we get the GSource handle rather
than the tag ID.

Meanwhile, caching the gsource in SocketChardev.telnet_source so that we
can also do dynamic context switch when update read handlers.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 8f0935cd15..a16d894c40 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -59,6 +59,7 @@ typedef struct {
     bool is_listen;
     bool is_telnet;
     bool is_tn3270;
+    GSource *telnet_source;
 
     GSource *reconnect_timer;
     int64_t reconnect_time;
@@ -69,6 +70,7 @@ typedef struct {
     OBJECT_CHECK(SocketChardev, (obj), TYPE_CHARDEV_SOCKET)
 
 static gboolean socket_reconnect_timeout(gpointer opaque);
+static void tcp_chr_telnet_init(Chardev *chr);
 
 static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
 {
@@ -555,6 +557,15 @@ static void tcp_chr_connect(void *opaque)
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
 
+static void tcp_chr_telnet_destroy(SocketChardev *s)
+{
+    if (s->telnet_source) {
+        g_source_destroy(s->telnet_source);
+        g_source_unref(s->telnet_source);
+        s->telnet_source = NULL;
+    }
+}
+
 static void tcp_chr_update_read_handler(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -568,6 +579,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
         qio_net_listener_set_context(s->listener, chr->gcontext);
     }
 
+    if (s->telnet_source) {
+        tcp_chr_telnet_destroy(s);
+        tcp_chr_telnet_init(CHARDEV(s));
+    }
+
     if (!s->connected) {
         return;
     }
@@ -592,6 +608,7 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
                                        gpointer user_data)
 {
     TCPChardevTelnetInit *init = user_data;
+    SocketChardev *s = SOCKET_CHARDEV(init->chr);
     ssize_t ret;
 
     ret = qio_channel_write(ioc, init->buf, init->buflen, NULL);
@@ -616,6 +633,8 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
 
 end:
     g_free(init);
+    g_source_unref(s->telnet_source);
+    s->telnet_source = NULL;
     return G_SOURCE_REMOVE;
 }
 
@@ -655,10 +674,10 @@ static void tcp_chr_telnet_init(Chardev *chr)
 
 #undef IACSET
 
-    qio_channel_add_watch(
-        s->ioc, G_IO_OUT,
-        tcp_chr_telnet_init_io,
-        init, NULL);
+    s->telnet_source = qio_channel_add_watch_full(s->ioc, G_IO_OUT,
+                                                  tcp_chr_telnet_init_io,
+                                                  init, NULL,
+                                                  chr->gcontext);
 }
 
 
@@ -831,6 +850,7 @@ static void char_socket_finalize(Object *obj)
     tcp_chr_free_connection(chr);
     tcp_chr_reconn_timer_cancel(s);
     qapi_free_SocketAddress(s->addr);
+    tcp_chr_telnet_destroy(s);
     if (s->listener) {
         qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
         object_unref(OBJECT(s->listener));
-- 
2.14.3

[Qemu-devel] [PATCH 09/14] qio: basic non-default context support for thread

[Peter Xu]

qio_task_run_in_thread() allows main thread to run blocking operations
in the background. However it has an assumption on that it's always
working with the default context. This patch tries to allow the QIO task
framework to run with non-default gcontext.

Currently no functional change so far, so the QIOTasks are still always
running on main context.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/io/task.h    |  6 ++++--
 io/channel-socket.c  |  9 ++++++---
 io/dns-resolver.c    |  3 ++-
 io/task.c            | 28 ++++++++++++++++++++++++++--
 tests/test-io-task.c |  2 ++
 5 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/include/io/task.h b/include/io/task.h
index 6021f51336..9dbe3758d7 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -227,15 +227,17 @@ QIOTask *qio_task_new(Object *source,
  * @worker: the function to invoke in a thread
  * @opaque: opaque data to pass to @worker
  * @destroy: function to free @opaque
+ * @context: the context to run the complete hook
  *
  * Run a task in a background thread. When @worker
  * returns it will call qio_task_complete() in
- * the main event thread context.
+ * the event thread context that provided.
  */
 void qio_task_run_in_thread(QIOTask *task,
                             QIOTaskWorker worker,
                             gpointer opaque,
-                            GDestroyNotify destroy);
+                            GDestroyNotify destroy,
+                            GMainContext *context);
 
 /**
  * qio_task_complete:
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 563e297357..4224ce323a 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -187,7 +187,8 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
     qio_task_run_in_thread(task,
                            qio_channel_socket_connect_worker,
                            addrCopy,
-                           (GDestroyNotify)qapi_free_SocketAddress);
+                           (GDestroyNotify)qapi_free_SocketAddress,
+                           NULL);
 }
 
 
@@ -245,7 +246,8 @@ void qio_channel_socket_listen_async(QIOChannelSocket *ioc,
     qio_task_run_in_thread(task,
                            qio_channel_socket_listen_worker,
                            addrCopy,
-                           (GDestroyNotify)qapi_free_SocketAddress);
+                           (GDestroyNotify)qapi_free_SocketAddress,
+                           NULL);
 }
 
 
@@ -321,7 +323,8 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
     qio_task_run_in_thread(task,
                            qio_channel_socket_dgram_worker,
                            data,
-                           qio_channel_socket_dgram_worker_free);
+                           qio_channel_socket_dgram_worker_free,
+                           NULL);
 }
 
 
diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index c072d121c3..75c2ca9c4a 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -233,7 +233,8 @@ void qio_dns_resolver_lookup_async(QIODNSResolver *resolver,
     qio_task_run_in_thread(task,
                            qio_dns_resolver_lookup_worker,
                            data,
-                           qio_dns_resolver_lookup_data_free);
+                           qio_dns_resolver_lookup_data_free,
+                           NULL);
 }
 
 
diff --git a/io/task.c b/io/task.c
index 1a0a1c7185..204c0be286 100644
--- a/io/task.c
+++ b/io/task.c
@@ -32,6 +32,10 @@ struct QIOTask {
     Error *err;
     gpointer result;
     GDestroyNotify destroyResult;
+
+    /* Threaded QIO task specific fields */
+    GSource *idle_source;  /* The idle task to run complete routine */
+    GMainContext *context; /* The context that idle task will run with */
 };
 
 
@@ -49,6 +53,7 @@ QIOTask *qio_task_new(Object *source,
     task->func = func;
     task->opaque = opaque;
     task->destroy = destroy;
+    task->idle_source = NULL;
 
     trace_qio_task_new(task, source, func, opaque);
 
@@ -66,6 +71,12 @@ static void qio_task_free(QIOTask *task)
     if (task->err) {
         error_free(task->err);
     }
+    if (task->idle_source) {
+        g_source_unref(task->idle_source);
+    }
+    if (task->context) {
+        g_main_context_unref(task->context);
+    }
     object_unref(task->source);
 
     g_free(task);
@@ -100,6 +111,8 @@ static gboolean qio_task_thread_result(gpointer opaque)
 static gpointer qio_task_thread_worker(gpointer opaque)
 {
     struct QIOTaskThreadData *data = opaque;
+    QIOTask *task = data->task;
+    GSource *idle;
 
     trace_qio_task_thread_run(data->task);
     data->worker(data->task, data->opaque);
@@ -110,7 +123,12 @@ static gpointer qio_task_thread_worker(gpointer opaque)
      * the worker results
      */
     trace_qio_task_thread_exit(data->task);
-    g_idle_add(qio_task_thread_result, data);
+
+    idle = g_idle_source_new();
+    g_source_set_callback(idle, qio_task_thread_result, data, NULL);
+    g_source_attach(idle, task->context);
+    task->idle_source = idle;
+
     return NULL;
 }
 
@@ -118,11 +136,17 @@ static gpointer qio_task_thread_worker(gpointer opaque)
 void qio_task_run_in_thread(QIOTask *task,
                             QIOTaskWorker worker,
                             gpointer opaque,
-                            GDestroyNotify destroy)
+                            GDestroyNotify destroy,
+                            GMainContext *context)
 {
     struct QIOTaskThreadData *data = g_new0(struct QIOTaskThreadData, 1);
     QemuThread thread;
 
+    if (context) {
+        g_main_context_ref(context);
+        task->context = context;
+    }
+
     data->task = task;
     data->worker = worker;
     data->opaque = opaque;
diff --git a/tests/test-io-task.c b/tests/test-io-task.c
index 141aa2c55d..bac1bb4e7a 100644
--- a/tests/test-io-task.c
+++ b/tests/test-io-task.c
@@ -187,6 +187,7 @@ static void test_task_thread_complete(void)
     qio_task_run_in_thread(task,
                            test_task_thread_worker,
                            &data,
+                           NULL,
                            NULL);
 
     g_main_loop_run(data.loop);
@@ -228,6 +229,7 @@ static void test_task_thread_failure(void)
     qio_task_run_in_thread(task,
                            test_task_thread_worker,
                            &data,
+                           NULL,
                            NULL);
 
     g_main_loop_run(data.loop);
-- 
2.14.3

[Qemu-devel] [PATCH 10/14] qio: refcount QIOTask

[Peter Xu]

It will be used in multiple threads in follow-up patches.  Let it start
to have refcounts.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/io/task.h |  3 +++
 io/task.c         | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/io/task.h b/include/io/task.h
index 9dbe3758d7..c6acd6489c 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
  */
 Object *qio_task_get_source(QIOTask *task);
 
+void qio_task_ref(QIOTask *task);
+void qio_task_unref(QIOTask *task);
+
 #endif /* QIO_TASK_H */
diff --git a/io/task.c b/io/task.c
index 204c0be286..00d3a5096a 100644
--- a/io/task.c
+++ b/io/task.c
@@ -32,6 +32,7 @@ struct QIOTask {
     Error *err;
     gpointer result;
     GDestroyNotify destroyResult;
+    uint32_t refcount;
 
     /* Threaded QIO task specific fields */
     GSource *idle_source;  /* The idle task to run complete routine */
@@ -57,6 +58,8 @@ QIOTask *qio_task_new(Object *source,
 
     trace_qio_task_new(task, source, func, opaque);
 
+    qio_task_ref(task);
+
     return task;
 }
 
@@ -165,7 +168,7 @@ void qio_task_complete(QIOTask *task)
 {
     task->func(task, task->opaque);
     trace_qio_task_complete(task);
-    qio_task_free(task);
+    qio_task_unref(task);
 }
 
 
@@ -208,3 +211,21 @@ Object *qio_task_get_source(QIOTask *task)
 {
     return task->source;
 }
+
+void qio_task_ref(QIOTask *task)
+{
+    if (!task) {
+        return;
+    }
+    atomic_inc(&task->refcount);
+}
+
+void qio_task_unref(QIOTask *task)
+{
+    if (!task) {
+        return;
+    }
+    if (atomic_fetch_dec(&task->refcount) == 1) {
+        qio_task_free(task);
+    }
+}
-- 
2.14.3

[Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async

[Peter Xu]

Let qio_channel_socket_connect_async() return the created QIOTask object
for the async connection.  In tcp chardev, cache that in SocketChardev
for further use.  With the QIOTask refcount, this is pretty safe.

Since at it, generalize out tcp_chr_socket_connect_async() since the
logic is used in both initial phase and reconnect timeout.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c       | 33 ++++++++++++++++++++++-----------
 include/io/channel-socket.h | 14 +++++++++-----
 io/channel-socket.c         | 12 +++++++-----
 3 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index a16d894c40..9d51b8da07 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -64,6 +64,7 @@ typedef struct {
     GSource *reconnect_timer;
     int64_t reconnect_time;
     bool connect_err_reported;
+    QIOTask *thread_task;
 } SocketChardev;
 
 #define SOCKET_CHARDEV(obj)                                     \
@@ -879,14 +880,32 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque)
     tcp_chr_new_client(chr, sioc);
 
 cleanup:
+    assert(s->thread_task == task);
+    qio_task_unref(task);
+    s->thread_task = NULL;
     object_unref(OBJECT(sioc));
 }
 
+static void tcp_chr_socket_connect_async(SocketChardev *s)
+{
+    QIOChannelSocket *sioc = qio_channel_socket_new();
+    Chardev *chr = CHARDEV(s);
+    QIOTask *task;
+
+    assert(s->thread_task == NULL);
+
+    tcp_chr_set_client_ioc_name(chr, sioc);
+    task = qio_channel_socket_connect_async(sioc, s->addr,
+                                            qemu_chr_socket_connected,
+                                            chr, NULL);
+    qio_task_ref(task);
+    s->thread_task = task;
+}
+
 static gboolean socket_reconnect_timeout(gpointer opaque)
 {
     Chardev *chr = CHARDEV(opaque);
     SocketChardev *s = SOCKET_CHARDEV(opaque);
-    QIOChannelSocket *sioc;
 
     g_source_unref(s->reconnect_timer);
     s->reconnect_timer = NULL;
@@ -895,11 +914,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
         return false;
     }
 
-    sioc = qio_channel_socket_new();
-    tcp_chr_set_client_ioc_name(chr, sioc);
-    qio_channel_socket_connect_async(sioc, s->addr,
-                                     qemu_chr_socket_connected,
-                                     chr, NULL);
+    tcp_chr_socket_connect_async(s);
 
     return false;
 }
@@ -979,11 +994,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
     }
 
     if (s->reconnect_time) {
-        sioc = qio_channel_socket_new();
-        tcp_chr_set_client_ioc_name(chr, sioc);
-        qio_channel_socket_connect_async(sioc, s->addr,
-                                         qemu_chr_socket_connected,
-                                         chr, NULL);
+        tcp_chr_socket_connect_async(s);
     } else {
         if (s->is_listen) {
             char *name;
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index 53801f6042..5cfa9e2b7c 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -108,12 +108,16 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
  * will be invoked on completion or failure. The @addr
  * parameter will be copied, so may be freed as soon
  * as this function returns without waiting for completion.
+ *
+ * Returns the IOTask created.  NOTE: if the caller is going to use
+ * the returned QIOTask, the caller is responsible to reference the
+ * task and unref it when it's not needed any more.
  */
-void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
-                                      SocketAddress *addr,
-                                      QIOTaskFunc callback,
-                                      gpointer opaque,
-                                      GDestroyNotify destroy);
+QIOTask *qio_channel_socket_connect_async(QIOChannelSocket *ioc,
+                                          SocketAddress *addr,
+                                          QIOTaskFunc callback,
+                                          gpointer opaque,
+                                          GDestroyNotify destroy);
 
 
 /**
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 4224ce323a..f420502290 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -169,11 +169,11 @@ static void qio_channel_socket_connect_worker(QIOTask *task,
 }
 
 
-void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
-                                      SocketAddress *addr,
-                                      QIOTaskFunc callback,
-                                      gpointer opaque,
-                                      GDestroyNotify destroy)
+QIOTask *qio_channel_socket_connect_async(QIOChannelSocket *ioc,
+                                          SocketAddress *addr,
+                                          QIOTaskFunc callback,
+                                          gpointer opaque,
+                                          GDestroyNotify destroy)
 {
     QIOTask *task = qio_task_new(
         OBJECT(ioc), callback, opaque, destroy);
@@ -189,6 +189,8 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
                            addrCopy,
                            (GDestroyNotify)qapi_free_SocketAddress,
                            NULL);
+
+    return task;
 }
 
 
-- 
2.14.3

[Qemu-devel] [PATCH 12/14] qio: move QIOTaskThreadData into QIOTask

[Peter Xu]

The major reason to do this is that, after the upper level can cache the
QIOTask, it should also be able to further manage the QIOTask.  And, it
can't if it does not have the information in QIOTaskThreadData.  So
let's just merge this struct with QIOTask.  Actually by doing this,
it'll simplify the code a bit too.

This will be needed in the next patch, when we want to rebuild the
completion GSource when the GMainContext changed.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 io/task.c | 46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/io/task.c b/io/task.c
index 00d3a5096a..080f9560ea 100644
--- a/io/task.c
+++ b/io/task.c
@@ -24,6 +24,13 @@
 #include "qemu/thread.h"
 #include "trace.h"
 
+struct QIOTaskThreadData {
+    QIOTaskWorker worker;
+    gpointer opaque;
+    GDestroyNotify destroy;
+};
+typedef struct QIOTaskThreadData QIOTaskThreadData;
+
 struct QIOTask {
     Object *source;
     QIOTaskFunc func;
@@ -37,6 +44,7 @@ struct QIOTask {
     /* Threaded QIO task specific fields */
     GSource *idle_source;  /* The idle task to run complete routine */
     GMainContext *context; /* The context that idle task will run with */
+    QIOTaskThreadData thread_data;
 };
 
 
@@ -86,26 +94,25 @@ static void qio_task_free(QIOTask *task)
 }
 
 
-struct QIOTaskThreadData {
-    QIOTask *task;
-    QIOTaskWorker worker;
-    gpointer opaque;
-    GDestroyNotify destroy;
-};
-
-
 static gboolean qio_task_thread_result(gpointer opaque)
 {
-    struct QIOTaskThreadData *data = opaque;
+    QIOTask *task = opaque;
+    QIOTaskThreadData *data = &task->thread_data;
 
-    trace_qio_task_thread_result(data->task);
-    qio_task_complete(data->task);
+    /*
+     * Take one more refcount since qio_task_complete() may otherwise
+     * release the last refcount and free, then "data" may be invalid.
+     */
+    qio_task_ref(task);
+
+    trace_qio_task_thread_result(task);
+    qio_task_complete(task);
 
     if (data->destroy) {
         data->destroy(data->opaque);
     }
 
-    g_free(data);
+    qio_task_unref(task);
 
     return FALSE;
 }
@@ -113,19 +120,19 @@ static gboolean qio_task_thread_result(gpointer opaque)
 
 static gpointer qio_task_thread_worker(gpointer opaque)
 {
-    struct QIOTaskThreadData *data = opaque;
-    QIOTask *task = data->task;
+    QIOTask *task = opaque;
+    QIOTaskThreadData *data = &task->thread_data;
     GSource *idle;
 
-    trace_qio_task_thread_run(data->task);
-    data->worker(data->task, data->opaque);
+    trace_qio_task_thread_run(task);
+    data->worker(task, data->opaque);
 
     /* We're running in the background thread, and must only
      * ever report the task results in the main event loop
      * thread. So we schedule an idle callback to report
      * the worker results
      */
-    trace_qio_task_thread_exit(data->task);
+    trace_qio_task_thread_exit(task);
 
     idle = g_idle_source_new();
     g_source_set_callback(idle, qio_task_thread_result, data, NULL);
@@ -142,15 +149,14 @@ void qio_task_run_in_thread(QIOTask *task,
                             GDestroyNotify destroy,
                             GMainContext *context)
 {
-    struct QIOTaskThreadData *data = g_new0(struct QIOTaskThreadData, 1);
     QemuThread thread;
+    QIOTaskThreadData *data = &task->thread_data;
 
     if (context) {
         g_main_context_ref(context);
         task->context = context;
     }
 
-    data->task = task;
     data->worker = worker;
     data->opaque = opaque;
     data->destroy = destroy;
@@ -159,7 +165,7 @@ void qio_task_run_in_thread(QIOTask *task,
     qemu_thread_create(&thread,
                        "io-task-worker",
                        qio_task_thread_worker,
-                       data,
+                       task,
                        QEMU_THREAD_DETACHED);
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts

[Peter Xu]

This is the part of work to allow the QIOTask to use a different
gcontext rather than the default main gcontext, by providing
qio_task_context_set() API.

We have done some work before on doing similar things to add non-default
gcontext support.  The general idea is that we delete the old GSource
from the main context, then re-add a new one to the new context when
context changed to a non-default one.  However this trick won't work
easily for threaded QIOTasks since we can't easily stop a real thread
and re-setup the whole thing from the very beginning.

But luckily, we don't need to do anything to the thread.  We just need
to keep an eye on the GSource that completes the QIOTask, which is
assigned to gcontext after the sync operation finished.

So when we setup a non-default GMainContext for a threaded QIO task, we
may face two cases:

- the thread is still running the sync task: then we don't need to do
  anything, only to update QIOTask.context to the new context

- the thread has finished the sync task and queued an idle task to main
  thread: then we destroy that old idle task, and re-create it on the
  new GMainContext.

Note that along the way when we modify either idle GSource or the
context, we need to take the mutex before hand, since the thread may be
modifying them at the same time.

Finally, call qio_task_context_set() in the tcp chardev update read
handler hook if QIOTask is running.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c |  4 +++
 include/io/task.h     |  1 +
 io/task.c             | 70 ++++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 9d51b8da07..164a64ff34 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -585,6 +585,10 @@ static void tcp_chr_update_read_handler(Chardev *chr)
         tcp_chr_telnet_init(CHARDEV(s));
     }
 
+    if (s->thread_task) {
+        qio_task_context_set(s->thread_task, chr->gcontext);
+    }
+
     if (!s->connected) {
         return;
     }
diff --git a/include/io/task.h b/include/io/task.h
index c6acd6489c..87e0152d8a 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -324,5 +324,6 @@ Object *qio_task_get_source(QIOTask *task);
 
 void qio_task_ref(QIOTask *task);
 void qio_task_unref(QIOTask *task);
+void qio_task_context_set(QIOTask *task, GMainContext *context);
 
 #endif /* QIO_TASK_H */
diff --git a/io/task.c b/io/task.c
index 080f9560ea..59bc439bdf 100644
--- a/io/task.c
+++ b/io/task.c
@@ -42,6 +42,9 @@ struct QIOTask {
     uint32_t refcount;
 
     /* Threaded QIO task specific fields */
+    bool has_thread;
+    QemuThread thread;
+    QemuMutex mutex;       /* Protects threaded QIO task fields */
     GSource *idle_source;  /* The idle task to run complete routine */
     GMainContext *context; /* The context that idle task will run with */
     QIOTaskThreadData thread_data;
@@ -57,6 +60,8 @@ QIOTask *qio_task_new(Object *source,
 
     task = g_new0(QIOTask, 1);
 
+    qemu_mutex_init(&task->mutex);
+
     task->source = source;
     object_ref(source);
     task->func = func;
@@ -88,7 +93,16 @@ static void qio_task_free(QIOTask *task)
     if (task->context) {
         g_main_context_unref(task->context);
     }
+    /*
+     * Make sure the thread quitted before we destroy the mutex,
+     * otherwise the thread might still be using it.
+     */
+    if (task->has_thread) {
+        qemu_thread_join(&task->thread);
+    }
+
     object_unref(task->source);
+    qemu_mutex_destroy(&task->mutex);
 
     g_free(task);
 }
@@ -117,12 +131,28 @@ static gboolean qio_task_thread_result(gpointer opaque)
     return FALSE;
 }
 
+/* Must be with QIOTask.mutex held. */
+static void qio_task_thread_create_complete_job(QIOTask *task)
+{
+    GSource *idle;
+
+    /* Remove the old if there is */
+    if (task->idle_source) {
+        g_source_destroy(task->idle_source);
+        g_source_unref(task->idle_source);
+    }
+
+    idle = g_idle_source_new();
+    g_source_set_callback(idle, qio_task_thread_result, task, NULL);
+    g_source_attach(idle, task->context);
+
+    task->idle_source = idle;
+}
 
 static gpointer qio_task_thread_worker(gpointer opaque)
 {
     QIOTask *task = opaque;
     QIOTaskThreadData *data = &task->thread_data;
-    GSource *idle;
 
     trace_qio_task_thread_run(task);
     data->worker(task, data->opaque);
@@ -134,10 +164,9 @@ static gpointer qio_task_thread_worker(gpointer opaque)
      */
     trace_qio_task_thread_exit(task);
 
-    idle = g_idle_source_new();
-    g_source_set_callback(idle, qio_task_thread_result, data, NULL);
-    g_source_attach(idle, task->context);
-    task->idle_source = idle;
+    qemu_mutex_lock(&task->mutex);
+    qio_task_thread_create_complete_job(task);
+    qemu_mutex_unlock(&task->mutex);
 
     return NULL;
 }
@@ -149,24 +178,21 @@ void qio_task_run_in_thread(QIOTask *task,
                             GDestroyNotify destroy,
                             GMainContext *context)
 {
-    QemuThread thread;
     QIOTaskThreadData *data = &task->thread_data;
 
-    if (context) {
-        g_main_context_ref(context);
-        task->context = context;
-    }
+    qio_task_context_set(task, context);
 
     data->worker = worker;
     data->opaque = opaque;
     data->destroy = destroy;
 
     trace_qio_task_thread_start(task, worker, opaque);
-    qemu_thread_create(&thread,
+    qemu_thread_create(&task->thread,
                        "io-task-worker",
                        qio_task_thread_worker,
                        task,
-                       QEMU_THREAD_DETACHED);
+                       QEMU_THREAD_JOINABLE);
+    task->has_thread = true;
 }
 
 
@@ -235,3 +261,23 @@ void qio_task_unref(QIOTask *task)
         qio_task_free(task);
     }
 }
+
+void qio_task_context_set(QIOTask *task, GMainContext *context)
+{
+    qemu_mutex_lock(&task->mutex);
+    if (task->context) {
+        g_main_context_unref(task->context);
+    }
+    if (context) {
+        g_main_context_ref(task->context);
+        task->context = context;
+    }
+    if (task->idle_source) {
+        /*
+         * We have had an idle job on the old context. Firstly delete
+         * the old one, then create a new one on the new context.
+         */
+        qio_task_thread_create_complete_job(task);
+    }
+    qemu_mutex_unlock(&task->mutex);
+}
-- 
2.14.3

[Qemu-devel] [PATCH 14/14] qio/chardev: specify gcontext for TLS handshake

[Peter Xu]

We allow the TLS code to be run with non-default gcontext by providing a
new qio_channel_tls_handshake_full() API.

With the new API, we can re-setup the TLS handshake GSource by calling
it again with the correct gcontext.  Any call to the function will clean
up existing GSource tasks, and re-setup using the new gcontext.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c    | 30 +++++++++++++---
 include/io/channel-tls.h | 22 +++++++++++-
 io/channel-tls.c         | 91 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 123 insertions(+), 20 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 164a64ff34..406d33c04f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -72,6 +72,9 @@ typedef struct {
 
 static gboolean socket_reconnect_timeout(gpointer opaque);
 static void tcp_chr_telnet_init(Chardev *chr);
+static void tcp_chr_tls_handshake_setup(Chardev *chr,
+                                        QIOChannelTLS *tioc,
+                                        GMainContext *context);
 
 static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
 {
@@ -570,6 +573,7 @@ static void tcp_chr_telnet_destroy(SocketChardev *s)
 static void tcp_chr_update_read_handler(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
+    QIOChannelTLS *tioc;
 
     if (s->listener) {
         /*
@@ -589,6 +593,17 @@ static void tcp_chr_update_read_handler(Chardev *chr)
         qio_task_context_set(s->thread_task, chr->gcontext);
     }
 
+    tioc = (QIOChannelTLS *)object_dynamic_cast(OBJECT(s->ioc),
+                                                TYPE_QIO_CHANNEL_TLS);
+    if (tioc) {
+        /*
+         * TLS session enabled; reconfigure things up.  Note that, if
+         * there is existing handshake task, it'll be cleaned up first
+         * in QIO code.
+         */
+        tcp_chr_tls_handshake_setup(chr, tioc, chr->gcontext);
+    }
+
     if (!s->connected) {
         return;
     }
@@ -704,6 +719,16 @@ static void tcp_chr_tls_handshake(QIOTask *task,
     }
 }
 
+static void tcp_chr_tls_handshake_setup(Chardev *chr,
+                                        QIOChannelTLS *tioc,
+                                        GMainContext *context)
+{
+    qio_channel_tls_handshake_full(tioc,
+                                   tcp_chr_tls_handshake,
+                                   chr,
+                                   NULL,
+                                   context);
+}
 
 static void tcp_chr_tls_init(Chardev *chr)
 {
@@ -736,10 +761,7 @@ static void tcp_chr_tls_init(Chardev *chr)
     object_unref(OBJECT(s->ioc));
     s->ioc = QIO_CHANNEL(tioc);
 
-    qio_channel_tls_handshake(tioc,
-                              tcp_chr_tls_handshake,
-                              chr,
-                              NULL);
+    tcp_chr_tls_handshake_setup(chr, tioc, NULL);
 }
 
 
diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
index d157eb10e8..98b7cd1e51 100644
--- a/include/io/channel-tls.h
+++ b/include/io/channel-tls.h
@@ -48,6 +48,9 @@ struct QIOChannelTLS {
     QIOChannel parent;
     QIOChannel *master;
     QCryptoTLSSession *session;
+    GMainContext *context;
+    GSource *tls_source;
+    QIOTask *task;
 };
 
 /**
@@ -111,11 +114,12 @@ qio_channel_tls_new_client(QIOChannel *master,
                            Error **errp);
 
 /**
- * qio_channel_tls_handshake:
+ * qio_channel_tls_handshake_full:
  * @ioc: the TLS channel object
  * @func: the callback to invoke when completed
  * @opaque: opaque data to pass to @func
  * @destroy: optional callback to free @opaque
+ * @context: the context that will run the handshake task
  *
  * Perform the TLS session handshake. This method
  * will return immediately and the handshake will
@@ -123,6 +127,22 @@ qio_channel_tls_new_client(QIOChannel *master,
  * loop is running. When the handshake is complete,
  * or fails, the @func callback will be invoked.
  */
+void qio_channel_tls_handshake_full(QIOChannelTLS *ioc,
+                                    QIOTaskFunc func,
+                                    gpointer opaque,
+                                    GDestroyNotify destroy,
+                                    GMainContext *context);
+
+/**
+ * qio_channel_tls_handshake:
+ * @ioc: the TLS channel object
+ * @func: the callback to invoke when completed
+ * @opaque: opaque data to pass to @func
+ * @destroy: optional callback to free @opaque
+ *
+ * Wrapper of qio_channel_tls_handshake_full(), only that we are
+ * running the handshake always on default main context.
+ */
 void qio_channel_tls_handshake(QIOChannelTLS *ioc,
                                QIOTaskFunc func,
                                gpointer opaque,
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 6182702dab..b173680526 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -145,8 +145,12 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
                                              GIOCondition condition,
                                              gpointer user_data);
 
-static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
-                                           QIOTask *task)
+/*
+ * Returns NULL if handshake completed, or a GSource pointer of the
+ * pending handshake task to be executed.
+ */
+static GSource *qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
+                                               QIOTask *task)
 {
     Error *err = NULL;
     QCryptoTLSSessionHandshakeStatus status;
@@ -155,7 +159,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
         trace_qio_channel_tls_handshake_fail(ioc);
         qio_task_set_error(task, err);
         qio_task_complete(task);
-        return;
+        return NULL;
     }
 
     status = qcrypto_tls_session_get_handshake_status(ioc->session);
@@ -169,6 +173,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
             trace_qio_channel_tls_credentials_allow(ioc);
         }
         qio_task_complete(task);
+        return NULL;
     } else {
         GIOCondition condition;
         if (status == QCRYPTO_TLS_HANDSHAKE_SENDING) {
@@ -178,14 +183,36 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
         }
 
         trace_qio_channel_tls_handshake_pending(ioc, status);
-        qio_channel_add_watch(ioc->master,
-                              condition,
-                              qio_channel_tls_handshake_io,
-                              task,
-                              NULL);
+        return qio_channel_add_watch_full(ioc->master,
+                                          condition,
+                                          qio_channel_tls_handshake_io,
+                                          task,
+                                          NULL,
+                                          ioc->context);
+    }
+}
+
+static void qio_channel_tls_context_set(QIOChannelTLS *ioc,
+                                        GMainContext *context)
+{
+    if (ioc->context) {
+        g_main_context_unref(ioc->context);
+        ioc->context = NULL;
+    }
+    if (context) {
+        g_main_context_ref(context);
+        ioc->context = context;
     }
 }
 
+static void qio_channel_tls_source_destroy(QIOChannelTLS *ioc)
+{
+    if (ioc->tls_source) {
+        g_source_destroy(ioc->tls_source);
+        g_source_unref(ioc->tls_source);
+        ioc->tls_source = NULL;
+    }
+}
 
 static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
                                              GIOCondition condition,
@@ -194,27 +221,59 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
     QIOTask *task = user_data;
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(
         qio_task_get_source(task));
+    GSource *source;
 
-    qio_channel_tls_handshake_task(
-       tioc, task);
+    source = qio_channel_tls_handshake_task(tioc, task);
+    /* Release existing GSource and cache the new one */
+    g_source_unref(tioc->tls_source);
+    tioc->tls_source = source;
 
     return FALSE;
 }
 
-void qio_channel_tls_handshake(QIOChannelTLS *ioc,
-                               QIOTaskFunc func,
-                               gpointer opaque,
-                               GDestroyNotify destroy)
+static void qio_channel_tls_cleanup(QIOChannelTLS *ioc)
+{
+    if (ioc->task) {
+        qio_task_unref(ioc->task);
+        ioc->task = NULL;
+    }
+
+    qio_channel_tls_source_destroy(ioc);
+    qio_channel_tls_context_set(ioc, NULL);
+}
+
+void qio_channel_tls_handshake_full(QIOChannelTLS *ioc,
+                                    QIOTaskFunc func,
+                                    gpointer opaque,
+                                    GDestroyNotify destroy,
+                                    GMainContext *context)
 {
     QIOTask *task;
+    GSource *source;
+
+    /* Drop existing tasks if there is */
+    qio_channel_tls_cleanup(ioc);
 
     task = qio_task_new(OBJECT(ioc),
                         func, opaque, destroy);
+    qio_task_ref(ioc->task);
+    ioc->task = task;
 
     trace_qio_channel_tls_handshake_start(ioc);
-    qio_channel_tls_handshake_task(ioc, task);
+
+    assert(ioc->tls_source == NULL);
+    qio_channel_tls_context_set(ioc, context);
+    source = qio_channel_tls_handshake_task(ioc, task);
+    ioc->tls_source = source;
 }
 
+void qio_channel_tls_handshake(QIOChannelTLS *ioc,
+                               QIOTaskFunc func,
+                               gpointer opaque,
+                               GDestroyNotify destroy)
+{
+    qio_channel_tls_handshake_full(ioc, func, opaque, destroy, NULL);
+}
 
 static void qio_channel_tls_init(Object *obj G_GNUC_UNUSED)
 {
@@ -225,6 +284,8 @@ static void qio_channel_tls_finalize(Object *obj)
 {
     QIOChannelTLS *ioc = QIO_CHANNEL_TLS(obj);
 
+    qio_channel_tls_source_destroy(ioc);
+    qio_channel_tls_context_set(ioc, NULL);
     object_unref(OBJECT(ioc->master));
     qcrypto_tls_session_free(ioc->session);
 }
-- 
2.14.3

Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote:
> It's a more powerful version of qio_channel_add_watch(), which supports
> non-default gcontext.  It's stripped from the old one, then we have
> g_source_get_id() to fetch the tag ID to keep the old interface.
> 
> Note that the new API will return a gsource, meanwhile keep a reference
> of it so that callers need to unref them explicitly.

I don't really like this. Having qio_channel_add_watch and
qio_channel_add_watch_full with differing return values is
really very surprising. They should be functionally identical,
except for the extra context arg.

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/io/channel.h | 31 ++++++++++++++++++++++++++++---
>  io/channel.c         | 24 +++++++++++++++++++-----
>  2 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 3995e243a3..36af5e58ae 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -620,20 +620,45 @@ GSource *qio_channel_create_watch(QIOChannel *ioc,
>                                    GIOCondition condition);
>  
>  /**
> - * qio_channel_add_watch:
> + * qio_channel_add_watch_full:
>   * @ioc: the channel object
>   * @condition: the I/O condition to monitor
>   * @func: callback to invoke when the source becomes ready
>   * @user_data: opaque data to pass to @func
>   * @notify: callback to free @user_data
> + * @context: gcontext to bind the source to
>   *
> - * Create a new main loop source that is used to watch
> + * Create a new source that is used to watch
>   * for the I/O condition @condition. The callback @func
>   * will be registered against the source, to be invoked
>   * when the source becomes ready. The optional @user_data
>   * will be passed to @func when it is invoked. The @notify
>   * callback will be used to free @user_data when the
> - * watch is deleted
> + * watch is deleted.  The source will be bound to @context if
> + * provided, or main context if it is NULL.
> + *
> + * Note: if a valid source is returned, we need to explicitly unref
> + * the source to destroy it.
> + *
> + * Returns: the source pointer
> + */
> +GSource *qio_channel_add_watch_full(QIOChannel *ioc,
> +                                    GIOCondition condition,
> +                                    QIOChannelFunc func,
> +                                    gpointer user_data,
> +                                    GDestroyNotify notify,
> +                                    GMainContext *context);
> +
> +/**
> + * qio_channel_add_watch:
> + * @ioc: the channel object
> + * @condition: the I/O condition to monitor
> + * @func: callback to invoke when the source becomes ready
> + * @user_data: opaque data to pass to @func
> + * @notify: callback to free @user_data
> + *
> + * Wrapper of qio_channel_add_watch_full(), but it'll only bind the
> + * source object to default main context.
>   *
>   * The returned source ID can be used with g_source_remove()
>   * to remove and free the source when no longer required.
> diff --git a/io/channel.c b/io/channel.c
> index ec4b86de7c..3e734cc9a5 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -299,6 +299,22 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc,
>      klass->io_set_aio_fd_handler(ioc, ctx, io_read, io_write, opaque);
>  }
>  
> +GSource *qio_channel_add_watch_full(QIOChannel *ioc,
> +                                    GIOCondition condition,
> +                                    QIOChannelFunc func,
> +                                    gpointer user_data,
> +                                    GDestroyNotify notify,
> +                                    GMainContext *context)
> +{
> +    GSource *source;
> +
> +    source = qio_channel_create_watch(ioc, condition);
> +    g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
> +    g_source_attach(source, context);
> +
> +    return source;
> +}
> +
>  guint qio_channel_add_watch(QIOChannel *ioc,
>                              GIOCondition condition,
>                              QIOChannelFunc func,
> @@ -308,11 +324,9 @@ guint qio_channel_add_watch(QIOChannel *ioc,
>      GSource *source;
>      guint id;
>  
> -    source = qio_channel_create_watch(ioc, condition);
> -
> -    g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
> -
> -    id = g_source_attach(source, NULL);
> +    source = qio_channel_add_watch_full(ioc, condition, func,
> +                                        user_data, notify, NULL);
> +    id = g_source_get_id(source);
>      g_source_unref(source);
>  
>      return id;
> -- 
> 2.14.3
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 04/14] migration: let incoming side use thread context

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:23PM +0800, Peter Xu wrote:
> The old incoming migration is running in main thread and default
> gcontext.  With the new qio_channel_add_watch_full() we can now let it
> run in the thread's own gcontext (if there is one).
> 
> Currently this patch does nothing alone.  But when any of the incoming
> migration is run in another iothread (e.g., the upcoming migrate-recover
> command), this patch will bind the incoming logic to the iothread
> instead of the main thread (which may already get page faulted and
> hanged).
> 
> RDMA is not considered for now since it's not even using the QIO APIs at
> all.

Errm, yes, it is.

  struct QIOChannelRDMA {
    QIOChannel parent;
    RDMAContext *rdma;
    QEMUFile *file;
    size_t len;
    bool blocking; /* XXX we don't actually honour this yet */
  };
  ....

> 
> CC: Juan Quintela <quintela@redhat.com>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/exec.c   | 11 ++++++-----
>  migration/fd.c     | 11 ++++++-----
>  migration/socket.c | 12 +++++++-----
>  3 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/exec.c b/migration/exec.c
> index 0bc5a427dd..f401fc005e 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -55,6 +55,7 @@ void exec_start_incoming_migration(const char *command, Error **errp)
>  {
>      QIOChannel *ioc;
>      const char *argv[] = { "/bin/sh", "-c", command, NULL };
> +    GSource *source;
>  
>      trace_migration_exec_incoming(command);
>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> @@ -65,9 +66,9 @@ void exec_start_incoming_migration(const char *command, Error **errp)
>      }
>  
>      qio_channel_set_name(ioc, "migration-exec-incoming");
> -    qio_channel_add_watch(ioc,
> -                          G_IO_IN,
> -                          exec_accept_incoming_migration,
> -                          NULL,
> -                          NULL);
> +    source = qio_channel_add_watch_full(ioc, G_IO_IN,
> +                                        exec_accept_incoming_migration,
> +                                        NULL, NULL,
> +                                        g_main_context_get_thread_default());
> +    g_source_unref(source);
>  }
> diff --git a/migration/fd.c b/migration/fd.c
> index cd06182d1e..9c593eb3ff 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -55,6 +55,7 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
>  {
>      QIOChannel *ioc;
>      int fd;
> +    GSource *source;
>  
>      fd = strtol(infd, NULL, 0);
>      trace_migration_fd_incoming(fd);
> @@ -66,9 +67,9 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
>      }
>  
>      qio_channel_set_name(QIO_CHANNEL(ioc), "migration-fd-incoming");
> -    qio_channel_add_watch(ioc,
> -                          G_IO_IN,
> -                          fd_accept_incoming_migration,
> -                          NULL,
> -                          NULL);
> +    source = qio_channel_add_watch_full(ioc, G_IO_IN,
> +                                        fd_accept_incoming_migration,
> +                                        NULL, NULL,
> +                                        g_main_context_get_thread_default());
> +    g_source_unref(source);
>  }
> diff --git a/migration/socket.c b/migration/socket.c
> index e090097077..82c330083c 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -164,6 +164,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>                                              Error **errp)
>  {
>      QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> +    GSource *source;
>  
>      qio_channel_set_name(QIO_CHANNEL(listen_ioc),
>                           "migration-socket-listener");
> @@ -173,11 +174,12 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>          return;
>      }
>  
> -    qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
> -                          G_IO_IN,
> -                          socket_accept_incoming_migration,
> -                          listen_ioc,
> -                          (GDestroyNotify)object_unref);
> +    source = qio_channel_add_watch_full(QIO_CHANNEL(listen_ioc), G_IO_IN,
> +                                        socket_accept_incoming_migration,
> +                                        listen_ioc,
> +                                        (GDestroyNotify)object_unref,
> +                                        g_main_context_get_thread_default());
> +    g_source_unref(source);
>  }
>  
>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
> -- 
> 2.14.3
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote:
> It will be used in multiple threads in follow-up patches.  Let it start
> to have refcounts.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/io/task.h |  3 +++
>  io/task.c         | 23 ++++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/include/io/task.h b/include/io/task.h
> index 9dbe3758d7..c6acd6489c 100644
> --- a/include/io/task.h
> +++ b/include/io/task.h
> @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
>   */
>  Object *qio_task_get_source(QIOTask *task);
>  
> +void qio_task_ref(QIOTask *task);
> +void qio_task_unref(QIOTask *task);

It should just be turned back into a QObject as it was originally,
so we get refcounting for free.

> diff --git a/io/task.c b/io/task.c
> index 204c0be286..00d3a5096a 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -32,6 +32,7 @@ struct QIOTask {
>      Error *err;
>      gpointer result;
>      GDestroyNotify destroyResult;
> +    uint32_t refcount;
>  
>      /* Threaded QIO task specific fields */
>      GSource *idle_source;  /* The idle task to run complete routine */
> @@ -57,6 +58,8 @@ QIOTask *qio_task_new(Object *source,
>  
>      trace_qio_task_new(task, source, func, opaque);
>  
> +    qio_task_ref(task);
> +
>      return task;
>  }
>  
> @@ -165,7 +168,7 @@ void qio_task_complete(QIOTask *task)
>  {
>      task->func(task, task->opaque);
>      trace_qio_task_complete(task);
> -    qio_task_free(task);
> +    qio_task_unref(task);
>  }
>  
>  
> @@ -208,3 +211,21 @@ Object *qio_task_get_source(QIOTask *task)
>  {
>      return task->source;
>  }
> +
> +void qio_task_ref(QIOTask *task)
> +{
> +    if (!task) {
> +        return;
> +    }
> +    atomic_inc(&task->refcount);
> +}
> +
> +void qio_task_unref(QIOTask *task)
> +{
> +    if (!task) {
> +        return;
> +    }
> +    if (atomic_fetch_dec(&task->refcount) == 1) {
> +        qio_task_free(task);
> +    }
> +}
> -- 
> 2.14.3
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:30PM +0800, Peter Xu wrote:
> Let qio_channel_socket_connect_async() return the created QIOTask object
> for the async connection.  In tcp chardev, cache that in SocketChardev
> for further use.  With the QIOTask refcount, this is pretty safe.

Why do you want to return QIOTask ? This is going against the intended
design pattern for QIOTask (that was based on that in GLib). The task
supposed to be an internal use only helper that callers should never
be touching until the completion callback is invoked.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:32PM +0800, Peter Xu wrote:
> This is the part of work to allow the QIOTask to use a different
> gcontext rather than the default main gcontext, by providing
> qio_task_context_set() API.
> 
> We have done some work before on doing similar things to add non-default
> gcontext support.  The general idea is that we delete the old GSource
> from the main context, then re-add a new one to the new context when
> context changed to a non-default one.  However this trick won't work
> easily for threaded QIOTasks since we can't easily stop a real thread
> and re-setup the whole thing from the very beginning.

I think this entire usage pattern is really broken. We should not
provide a way to change the GMainContext on an existing task. We
should always just set the correct GMainContxt right from the start.
This will avoid much of the complexity you're introducing into this
patch series, and avoid having to expose GTasks to the callers of
the async methods at all.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:26PM +0800, Peter Xu wrote:
> TCP chardevs can be using QIO network listeners working in the
> background when in listening mode.  However the network listeners are
> always running in main context.  This can race with chardevs that are
> running in non-main contexts.
> 
> To solve this: firstly introduce qio_net_listener_set_context() to allow
> caller to set gcontext for network listeners.  Then call it in
> tcp_chr_update_read_handler(), with the newly cached gcontext.
> 
> It's fairly straightforward after we have introduced some net listener
> helper functions - basically we unregister the GSources and add them
> back with the correct context.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c     |  9 +++++++++
>  include/io/net-listener.h | 12 ++++++++++++
>  io/net-listener.c         |  7 +++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 43a2cc2c1c..8f0935cd15 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>  
> +    if (s->listener) {
> +        /*
> +         * It's possible that chardev context is changed in
> +         * qemu_chr_be_update_read_handlers().  Reset it for QIO net
> +         * listener if there is.
> +         */
> +        qio_net_listener_set_context(s->listener, chr->gcontext);
> +    }
> +
>      if (!s->connected) {
>          return;
>      }
> diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> index 566be283b3..39dede9d6f 100644
> --- a/include/io/net-listener.h
> +++ b/include/io/net-listener.h
> @@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
>                                 SocketAddress *addr,
>                                 Error **errp);
>  
> +/**
> + * qio_net_listener_set_context:
> + * @listener: the net listener object
> + * @context: the context that we'd like to bind the sources to
> + *
> + * This helper does not do anything but moves existing net listener
> + * sources from the old one to the new one.  It can be seen as a
> + * no-operation if there is no listening source at all.
> + */
> +void qio_net_listener_set_context(QIONetListener *listener,
> +                                  GMainContext *context);

I don't think this is a good design. The GMainContext should be provided
to the qio_net_listener_set_client_func method immediately, not updated
after the fact


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 01/14] chardev: fix leak in tcp_chr_telnet_init_io()

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:20PM +0800, Peter Xu wrote:
> Need to free TCPChardevTelnetInit when session established.
> 
> Since at it, switch to use G_SOURCE_* macros.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 02/14] qio: rename qio_task_thread_result

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:21PM +0800, Peter Xu wrote:
> It is strange that it was called gio_task_thread_result.  Rename it to
> follow the naming rule of the file.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  io/task.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/io/task.c b/io/task.c
> index 3ce556017c..1a0a1c7185 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -80,7 +80,7 @@ struct QIOTaskThreadData {
>  };
>  
>  
> -static gboolean gio_task_thread_result(gpointer opaque)
> +static gboolean qio_task_thread_result(gpointer opaque)
>  {
>      struct QIOTaskThreadData *data = opaque;
>  
> @@ -110,7 +110,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
>       * the worker results
>       */
>      trace_qio_task_thread_exit(data->task);
> -    g_idle_add(gio_task_thread_result, data);
> +    g_idle_add(qio_task_thread_result, data);
>      return NULL;
>  }
>  
> -- 
> 2.14.3
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()

[Peter Xu]

On Wed, Feb 28, 2018 at 09:08:45AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote:
> > It's a more powerful version of qio_channel_add_watch(), which supports
> > non-default gcontext.  It's stripped from the old one, then we have
> > g_source_get_id() to fetch the tag ID to keep the old interface.
> > 
> > Note that the new API will return a gsource, meanwhile keep a reference
> > of it so that callers need to unref them explicitly.
> 
> I don't really like this. Having qio_channel_add_watch and
> qio_channel_add_watch_full with differing return values is
> really very surprising. They should be functionally identical,
> except for the extra context arg.

Yeah it's not nice, but I do need the GSource and the tag ID does not
help in the series.

An alternative would be that I modify qio_channel_add_watch() to
return GSource too.  Is there an third choice that you could suggest?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 08:44:24PM +0800, Peter Xu wrote:
> On Wed, Feb 28, 2018 at 09:08:45AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote:
> > > It's a more powerful version of qio_channel_add_watch(), which supports
> > > non-default gcontext.  It's stripped from the old one, then we have
> > > g_source_get_id() to fetch the tag ID to keep the old interface.
> > > 
> > > Note that the new API will return a gsource, meanwhile keep a reference
> > > of it so that callers need to unref them explicitly.
> > 
> > I don't really like this. Having qio_channel_add_watch and
> > qio_channel_add_watch_full with differing return values is
> > really very surprising. They should be functionally identical,
> > except for the extra context arg.
> 
> Yeah it's not nice, but I do need the GSource and the tag ID does not
> help in the series.
> 
> An alternative would be that I modify qio_channel_add_watch() to
> return GSource too.  Is there an third choice that you could suggest?

Given you have the id + GMainContext you can just acquire the GSource,
if needed, using g_main_context_find_source_by_id.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext

[Peter Xu]

On Wed, Feb 28, 2018 at 09:25:11AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:26PM +0800, Peter Xu wrote:
> > TCP chardevs can be using QIO network listeners working in the
> > background when in listening mode.  However the network listeners are
> > always running in main context.  This can race with chardevs that are
> > running in non-main contexts.
> > 
> > To solve this: firstly introduce qio_net_listener_set_context() to allow
> > caller to set gcontext for network listeners.  Then call it in
> > tcp_chr_update_read_handler(), with the newly cached gcontext.
> > 
> > It's fairly straightforward after we have introduced some net listener
> > helper functions - basically we unregister the GSources and add them
> > back with the correct context.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  chardev/char-socket.c     |  9 +++++++++
> >  include/io/net-listener.h | 12 ++++++++++++
> >  io/net-listener.c         |  7 +++++++
> >  3 files changed, 28 insertions(+)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 43a2cc2c1c..8f0935cd15 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> >  
> > +    if (s->listener) {
> > +        /*
> > +         * It's possible that chardev context is changed in
> > +         * qemu_chr_be_update_read_handlers().  Reset it for QIO net
> > +         * listener if there is.
> > +         */
> > +        qio_net_listener_set_context(s->listener, chr->gcontext);
> > +    }
> > +
> >      if (!s->connected) {
> >          return;
> >      }
> > diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> > index 566be283b3..39dede9d6f 100644
> > --- a/include/io/net-listener.h
> > +++ b/include/io/net-listener.h
> > @@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
> >                                 SocketAddress *addr,
> >                                 Error **errp);
> >  
> > +/**
> > + * qio_net_listener_set_context:
> > + * @listener: the net listener object
> > + * @context: the context that we'd like to bind the sources to
> > + *
> > + * This helper does not do anything but moves existing net listener
> > + * sources from the old one to the new one.  It can be seen as a
> > + * no-operation if there is no listening source at all.
> > + */
> > +void qio_net_listener_set_context(QIONetListener *listener,
> > +                                  GMainContext *context);
> 
> I don't think this is a good design. The GMainContext should be provided
> to the qio_net_listener_set_client_func method immediately, not updated
> after the fact

After the patches, this is qio_net_listener_set_client_func_full():

void qio_net_listener_set_client_func_full(QIONetListener *listener,
                                           QIONetListenerClientFunc func,
                                           gpointer data,
                                           GDestroyNotify notify,
                                           GMainContext *context)
{
    if (listener->io_notify) {
        listener->io_notify(listener->io_data);
    }
    listener->io_func = func;
    listener->io_data = data;
    listener->io_notify = notify;

    qio_net_listener_sources_clear(listener);
    qio_net_listener_sources_update(listener, context);
}

This is qio_net_listener_set_context():

void qio_net_listener_set_context(QIONetListener *listener,
                                  GMainContext *context)
{
    qio_net_listener_sources_clear(listener);
    qio_net_listener_sources_update(listener, context);
}

So... Basically qio_net_listener_set_context() is just a simplified
version of qio_net_listener_set_client_func_full(), but without
passing in the funcs again.  So do you mean that I can just avoid
introducing this function and call
qio_net_listener_set_client_func_full() directly?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask

[Peter Xu]

On Wed, Feb 28, 2018 at 09:16:59AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote:
> > It will be used in multiple threads in follow-up patches.  Let it start
> > to have refcounts.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/io/task.h |  3 +++
> >  io/task.c         | 23 ++++++++++++++++++++++-
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/io/task.h b/include/io/task.h
> > index 9dbe3758d7..c6acd6489c 100644
> > --- a/include/io/task.h
> > +++ b/include/io/task.h
> > @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
> >   */
> >  Object *qio_task_get_source(QIOTask *task);
> >  
> > +void qio_task_ref(QIOTask *task);
> > +void qio_task_unref(QIOTask *task);
> 
> It should just be turned back into a QObject as it was originally,
> so we get refcounting for free.

Could you point me the commit that has the original code?  I would be
glad to revert to that, or yeah I can switch to QObject too.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()

[Peter Xu]

On Wed, Feb 28, 2018 at 12:47:43PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 08:44:24PM +0800, Peter Xu wrote:
> > On Wed, Feb 28, 2018 at 09:08:45AM +0000, Daniel P. Berrangé wrote:
> > > On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote:
> > > > It's a more powerful version of qio_channel_add_watch(), which supports
> > > > non-default gcontext.  It's stripped from the old one, then we have
> > > > g_source_get_id() to fetch the tag ID to keep the old interface.
> > > > 
> > > > Note that the new API will return a gsource, meanwhile keep a reference
> > > > of it so that callers need to unref them explicitly.
> > > 
> > > I don't really like this. Having qio_channel_add_watch and
> > > qio_channel_add_watch_full with differing return values is
> > > really very surprising. They should be functionally identical,
> > > except for the extra context arg.
> > 
> > Yeah it's not nice, but I do need the GSource and the tag ID does not
> > help in the series.
> > 
> > An alternative would be that I modify qio_channel_add_watch() to
> > return GSource too.  Is there an third choice that you could suggest?
> 
> Given you have the id + GMainContext you can just acquire the GSource,
> if needed, using g_main_context_find_source_by_id.

I always feel unsafe to play around with tag IDs since the IDs can
change after GSource removed and new GSource added, and also the
result of the call will depend on a correct pairing of context (so if
the context is incorrect, instead of failure, we possibly got
everything screwed up while we never know we failed...).

But indeed for this one it seems pretty safe if I call
g_main_context_find_source_by_id() right after I call
qio_channel_add_watch_full() to fetch the GSource.  If you agree, I
can use this approach in my next post.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts

[Peter Xu]

On Wed, Feb 28, 2018 at 09:23:56AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:32PM +0800, Peter Xu wrote:
> > This is the part of work to allow the QIOTask to use a different
> > gcontext rather than the default main gcontext, by providing
> > qio_task_context_set() API.
> > 
> > We have done some work before on doing similar things to add non-default
> > gcontext support.  The general idea is that we delete the old GSource
> > from the main context, then re-add a new one to the new context when
> > context changed to a non-default one.  However this trick won't work
> > easily for threaded QIOTasks since we can't easily stop a real thread
> > and re-setup the whole thing from the very beginning.
> 
> I think this entire usage pattern is really broken. We should not
> provide a way to change the GMainContext on an existing task. We
> should always just set the correct GMainContxt right from the start.
> This will avoid much of the complexity you're introducing into this
> patch series, and avoid having to expose GTasks to the callers of
> the async methods at all.

Yeah I agree with you that the threaded QIO patches are complicated.
Then how about I introduce:

- qio_task_thread_new(): to create QIO task and its thread (but not
                         running)
- qio_task_thread_run(): to run the thread

Then I can setup the context correctly between the two in some way.

After that, qio_task_run_in_thread() will be two calls of above two
APIs.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 08:52:16PM +0800, Peter Xu wrote:
> On Wed, Feb 28, 2018 at 09:25:11AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 01:06:26PM +0800, Peter Xu wrote:
> > > TCP chardevs can be using QIO network listeners working in the
> > > background when in listening mode.  However the network listeners are
> > > always running in main context.  This can race with chardevs that are
> > > running in non-main contexts.
> > > 
> > > To solve this: firstly introduce qio_net_listener_set_context() to allow
> > > caller to set gcontext for network listeners.  Then call it in
> > > tcp_chr_update_read_handler(), with the newly cached gcontext.
> > > 
> > > It's fairly straightforward after we have introduced some net listener
> > > helper functions - basically we unregister the GSources and add them
> > > back with the correct context.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  chardev/char-socket.c     |  9 +++++++++
> > >  include/io/net-listener.h | 12 ++++++++++++
> > >  io/net-listener.c         |  7 +++++++
> > >  3 files changed, 28 insertions(+)
> > > 
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 43a2cc2c1c..8f0935cd15 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> > >  {
> > >      SocketChardev *s = SOCKET_CHARDEV(chr);
> > >  
> > > +    if (s->listener) {
> > > +        /*
> > > +         * It's possible that chardev context is changed in
> > > +         * qemu_chr_be_update_read_handlers().  Reset it for QIO net
> > > +         * listener if there is.
> > > +         */
> > > +        qio_net_listener_set_context(s->listener, chr->gcontext);
> > > +    }
> > > +
> > >      if (!s->connected) {
> > >          return;
> > >      }
> > > diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> > > index 566be283b3..39dede9d6f 100644
> > > --- a/include/io/net-listener.h
> > > +++ b/include/io/net-listener.h
> > > @@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
> > >                                 SocketAddress *addr,
> > >                                 Error **errp);
> > >  
> > > +/**
> > > + * qio_net_listener_set_context:
> > > + * @listener: the net listener object
> > > + * @context: the context that we'd like to bind the sources to
> > > + *
> > > + * This helper does not do anything but moves existing net listener
> > > + * sources from the old one to the new one.  It can be seen as a
> > > + * no-operation if there is no listening source at all.
> > > + */
> > > +void qio_net_listener_set_context(QIONetListener *listener,
> > > +                                  GMainContext *context);
> > 
> > I don't think this is a good design. The GMainContext should be provided
> > to the qio_net_listener_set_client_func method immediately, not updated
> > after the fact
> 
> After the patches, this is qio_net_listener_set_client_func_full():
> 
> void qio_net_listener_set_client_func_full(QIONetListener *listener,
>                                            QIONetListenerClientFunc func,
>                                            gpointer data,
>                                            GDestroyNotify notify,
>                                            GMainContext *context)
> {
>     if (listener->io_notify) {
>         listener->io_notify(listener->io_data);
>     }
>     listener->io_func = func;
>     listener->io_data = data;
>     listener->io_notify = notify;
> 
>     qio_net_listener_sources_clear(listener);
>     qio_net_listener_sources_update(listener, context);
> }
> 
> This is qio_net_listener_set_context():
> 
> void qio_net_listener_set_context(QIONetListener *listener,
>                                   GMainContext *context)
> {
>     qio_net_listener_sources_clear(listener);
>     qio_net_listener_sources_update(listener, context);
> }
> 
> So... Basically qio_net_listener_set_context() is just a simplified
> version of qio_net_listener_set_client_func_full(), but without
> passing in the funcs again.  So do you mean that I can just avoid
> introducing this function and call
> qio_net_listener_set_client_func_full() directly?

Yes, I don't see any reason to allow GMainContext to be changed separately
from setting the callback functions. The caller can easily just call
qio_net_listener_set_client_func_full() directly with the new GMainContext

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 08:54:00PM +0800, Peter Xu wrote:
> On Wed, Feb 28, 2018 at 09:16:59AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote:
> > > It will be used in multiple threads in follow-up patches.  Let it start
> > > to have refcounts.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  include/io/task.h |  3 +++
> > >  io/task.c         | 23 ++++++++++++++++++++++-
> > >  2 files changed, 25 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/io/task.h b/include/io/task.h
> > > index 9dbe3758d7..c6acd6489c 100644
> > > --- a/include/io/task.h
> > > +++ b/include/io/task.h
> > > @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
> > >   */
> > >  Object *qio_task_get_source(QIOTask *task);
> > >  
> > > +void qio_task_ref(QIOTask *task);
> > > +void qio_task_unref(QIOTask *task);
> > 
> > It should just be turned back into a QObject as it was originally,
> > so we get refcounting for free.
> 
> Could you point me the commit that has the original code?  I would be
> glad to revert to that, or yeah I can switch to QObject too.  Thanks,

It was never actually committed - it was just that way during initial
review but was suggested to be simplified as ref counting wasn't needed.

That all said, having now seen the later parts of this patch series, I
don't think we would need todo this at all, because we should not be
exposing the GTask to callers and thus the refcounting question goes away

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async

[Peter Xu]

On Wed, Feb 28, 2018 at 09:20:47AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:30PM +0800, Peter Xu wrote:
> > Let qio_channel_socket_connect_async() return the created QIOTask object
> > for the async connection.  In tcp chardev, cache that in SocketChardev
> > for further use.  With the QIOTask refcount, this is pretty safe.
> 
> Why do you want to return QIOTask ? This is going against the intended
> design pattern for QIOTask (that was based on that in GLib). The task
> supposed to be an internal use only helper that callers should never
> be touching until the completion callback is invoked.

I proposed another solution in the other comment reply to split the
threaded QIO task into create() and run().  If you like that, I can
try.  Any other suggestion would be welcomed too.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask

[Peter Xu]

On Wed, Feb 28, 2018 at 01:07:44PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 08:54:00PM +0800, Peter Xu wrote:
> > On Wed, Feb 28, 2018 at 09:16:59AM +0000, Daniel P. Berrangé wrote:
> > > On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote:
> > > > It will be used in multiple threads in follow-up patches.  Let it start
> > > > to have refcounts.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  include/io/task.h |  3 +++
> > > >  io/task.c         | 23 ++++++++++++++++++++++-
> > > >  2 files changed, 25 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/io/task.h b/include/io/task.h
> > > > index 9dbe3758d7..c6acd6489c 100644
> > > > --- a/include/io/task.h
> > > > +++ b/include/io/task.h
> > > > @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
> > > >   */
> > > >  Object *qio_task_get_source(QIOTask *task);
> > > >  
> > > > +void qio_task_ref(QIOTask *task);
> > > > +void qio_task_unref(QIOTask *task);
> > > 
> > > It should just be turned back into a QObject as it was originally,
> > > so we get refcounting for free.
> > 
> > Could you point me the commit that has the original code?  I would be
> > glad to revert to that, or yeah I can switch to QObject too.  Thanks,
> 
> It was never actually committed - it was just that way during initial
> review but was suggested to be simplified as ref counting wasn't needed.
> 
> That all said, having now seen the later parts of this patch series, I
> don't think we would need todo this at all, because we should not be
> exposing the GTask to callers and thus the refcounting question goes away

Indeed.  I suppose that means you like my proposal in the other
thread.  But I'll wait for your explicit acknowledgement in that too.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 09:05:26PM +0800, Peter Xu wrote:
> On Wed, Feb 28, 2018 at 09:23:56AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 01:06:32PM +0800, Peter Xu wrote:
> > > This is the part of work to allow the QIOTask to use a different
> > > gcontext rather than the default main gcontext, by providing
> > > qio_task_context_set() API.
> > > 
> > > We have done some work before on doing similar things to add non-default
> > > gcontext support.  The general idea is that we delete the old GSource
> > > from the main context, then re-add a new one to the new context when
> > > context changed to a non-default one.  However this trick won't work
> > > easily for threaded QIOTasks since we can't easily stop a real thread
> > > and re-setup the whole thing from the very beginning.
> > 
> > I think this entire usage pattern is really broken. We should not
> > provide a way to change the GMainContext on an existing task. We
> > should always just set the correct GMainContxt right from the start.
> > This will avoid much of the complexity you're introducing into this
> > patch series, and avoid having to expose GTasks to the callers of
> > the async methods at all.
> 
> Yeah I agree with you that the threaded QIO patches are complicated.
> Then how about I introduce:
> 
> - qio_task_thread_new(): to create QIO task and its thread (but not
>                          running)
> - qio_task_thread_run(): to run the thread
> 
> Then I can setup the context correctly between the two in some way.
> 
> After that, qio_task_run_in_thread() will be two calls of above two
> APIs.

I don't see why it is not enough to just pass the right GMainContext
into qio_task_run_in_thread() immediately. There should not be any
need to split this into two parts. ALl that's needed here is for the
completion callback to rnu in the right context, which just means
passing the GMainContext from qio_task_run_in_thread, through to
qio_task_thread_worker via QIOTaskThreadData. Changing the GMainContext
after the thread is already created/running is not something we should
try todo.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 14/14] qio/chardev: specify gcontext for TLS handshake

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 01:06:33PM +0800, Peter Xu wrote:
> We allow the TLS code to be run with non-default gcontext by providing a
> new qio_channel_tls_handshake_full() API.
> 
> With the new API, we can re-setup the TLS handshake GSource by calling
> it again with the correct gcontext.  Any call to the function will clean
> up existing GSource tasks, and re-setup using the new gcontext.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c    | 30 +++++++++++++---
>  include/io/channel-tls.h | 22 +++++++++++-
>  io/channel-tls.c         | 91 ++++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 123 insertions(+), 20 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 164a64ff34..406d33c04f 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -72,6 +72,9 @@ typedef struct {
>  
>  static gboolean socket_reconnect_timeout(gpointer opaque);
>  static void tcp_chr_telnet_init(Chardev *chr);
> +static void tcp_chr_tls_handshake_setup(Chardev *chr,
> +                                        QIOChannelTLS *tioc,
> +                                        GMainContext *context);
>  
>  static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
>  {
> @@ -570,6 +573,7 @@ static void tcp_chr_telnet_destroy(SocketChardev *s)
>  static void tcp_chr_update_read_handler(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> +    QIOChannelTLS *tioc;
>  
>      if (s->listener) {
>          /*
> @@ -589,6 +593,17 @@ static void tcp_chr_update_read_handler(Chardev *chr)
>          qio_task_context_set(s->thread_task, chr->gcontext);
>      }
>  
> +    tioc = (QIOChannelTLS *)object_dynamic_cast(OBJECT(s->ioc),
> +                                                TYPE_QIO_CHANNEL_TLS);
> +    if (tioc) {
> +        /*
> +         * TLS session enabled; reconfigure things up.  Note that, if
> +         * there is existing handshake task, it'll be cleaned up first
> +         * in QIO code.
> +         */
> +        tcp_chr_tls_handshake_setup(chr, tioc, chr->gcontext);
> +    }

This is crazy - we should not be looking at specific implementations of
the channel. If the TLS object needs to use a specific GMainContext we
should make sure that is done right from the start and not try to change
the GMainContext on the fly.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 04/14] migration: let incoming side use thread context

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> The old incoming migration is running in main thread and default
> gcontext.  With the new qio_channel_add_watch_full() we can now let it
> run in the thread's own gcontext (if there is one).
> 
> Currently this patch does nothing alone.  But when any of the incoming
> migration is run in another iothread (e.g., the upcoming migrate-recover
> command), this patch will bind the incoming logic to the iothread
> instead of the main thread (which may already get page faulted and
> hanged).

Does this make any difference to the Postcopy listener thread, which
takes over reading from the main thread once in postcopy mode?
(See savevm.c:postcopy_ram_listen_thread).

Dave

> RDMA is not considered for now since it's not even using the QIO APIs at
> all.
> 
> CC: Juan Quintela <quintela@redhat.com>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/exec.c   | 11 ++++++-----
>  migration/fd.c     | 11 ++++++-----
>  migration/socket.c | 12 +++++++-----
>  3 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/exec.c b/migration/exec.c
> index 0bc5a427dd..f401fc005e 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -55,6 +55,7 @@ void exec_start_incoming_migration(const char *command, Error **errp)
>  {
>      QIOChannel *ioc;
>      const char *argv[] = { "/bin/sh", "-c", command, NULL };
> +    GSource *source;
>  
>      trace_migration_exec_incoming(command);
>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> @@ -65,9 +66,9 @@ void exec_start_incoming_migration(const char *command, Error **errp)
>      }
>  
>      qio_channel_set_name(ioc, "migration-exec-incoming");
> -    qio_channel_add_watch(ioc,
> -                          G_IO_IN,
> -                          exec_accept_incoming_migration,
> -                          NULL,
> -                          NULL);
> +    source = qio_channel_add_watch_full(ioc, G_IO_IN,
> +                                        exec_accept_incoming_migration,
> +                                        NULL, NULL,
> +                                        g_main_context_get_thread_default());
> +    g_source_unref(source);
>  }
> diff --git a/migration/fd.c b/migration/fd.c
> index cd06182d1e..9c593eb3ff 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -55,6 +55,7 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
>  {
>      QIOChannel *ioc;
>      int fd;
> +    GSource *source;
>  
>      fd = strtol(infd, NULL, 0);
>      trace_migration_fd_incoming(fd);
> @@ -66,9 +67,9 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
>      }
>  
>      qio_channel_set_name(QIO_CHANNEL(ioc), "migration-fd-incoming");
> -    qio_channel_add_watch(ioc,
> -                          G_IO_IN,
> -                          fd_accept_incoming_migration,
> -                          NULL,
> -                          NULL);
> +    source = qio_channel_add_watch_full(ioc, G_IO_IN,
> +                                        fd_accept_incoming_migration,
> +                                        NULL, NULL,
> +                                        g_main_context_get_thread_default());
> +    g_source_unref(source);
>  }
> diff --git a/migration/socket.c b/migration/socket.c
> index e090097077..82c330083c 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -164,6 +164,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>                                              Error **errp)
>  {
>      QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> +    GSource *source;
>  
>      qio_channel_set_name(QIO_CHANNEL(listen_ioc),
>                           "migration-socket-listener");
> @@ -173,11 +174,12 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>          return;
>      }
>  
> -    qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
> -                          G_IO_IN,
> -                          socket_accept_incoming_migration,
> -                          listen_ioc,
> -                          (GDestroyNotify)object_unref);
> +    source = qio_channel_add_watch_full(QIO_CHANNEL(listen_ioc), G_IO_IN,
> +                                        socket_accept_incoming_migration,
> +                                        listen_ioc,
> +                                        (GDestroyNotify)object_unref,
> +                                        g_main_context_get_thread_default());
> +    g_source_unref(source);
>  }
>  
>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 04/14] migration: let incoming side use thread context

[Peter Xu]

On Wed, Feb 28, 2018 at 05:43:50PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > The old incoming migration is running in main thread and default
> > gcontext.  With the new qio_channel_add_watch_full() we can now let it
> > run in the thread's own gcontext (if there is one).
> > 
> > Currently this patch does nothing alone.  But when any of the incoming
> > migration is run in another iothread (e.g., the upcoming migrate-recover
> > command), this patch will bind the incoming logic to the iothread
> > instead of the main thread (which may already get page faulted and
> > hanged).
> 
> Does this make any difference to the Postcopy listener thread, which
> takes over reading from the main thread once in postcopy mode?
> (See savevm.c:postcopy_ram_listen_thread).

It should not.  It should only affect when use sends a
"migrate-recover" with "run-oob=true".  The rest should be the same as
before.  And since the postcopy ram load thread is a standalone thread
with its own initial thread stack (so it's not really in a gmainloop),
I can hardly tell how that can be affected since it'll always use its
own thread stack.

Or, have I missed anything?

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 04/14] migration: let incoming side use thread context

[Peter Xu]

On Wed, Feb 28, 2018 at 09:10:58AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:23PM +0800, Peter Xu wrote:
> > The old incoming migration is running in main thread and default
> > gcontext.  With the new qio_channel_add_watch_full() we can now let it
> > run in the thread's own gcontext (if there is one).
> > 
> > Currently this patch does nothing alone.  But when any of the incoming
> > migration is run in another iothread (e.g., the upcoming migrate-recover
> > command), this patch will bind the incoming logic to the iothread
> > instead of the main thread (which may already get page faulted and
> > hanged).
> > 
> > RDMA is not considered for now since it's not even using the QIO APIs at
> > all.
> 
> Errm, yes, it is.
> 
>   struct QIOChannelRDMA {
>     QIOChannel parent;
>     RDMAContext *rdma;
>     QEMUFile *file;
>     size_t len;
>     bool blocking; /* XXX we don't actually honour this yet */
>   };
>   ....

Ah, you are right. :)

I should say that "it's not using QIO watch framework" since it's
using qemu_set_fd_handler() so it's always on main thread.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 14/14] qio/chardev: specify gcontext for TLS handshake

[Peter Xu]

On Wed, Feb 28, 2018 at 01:22:37PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:33PM +0800, Peter Xu wrote:
> > We allow the TLS code to be run with non-default gcontext by providing a
> > new qio_channel_tls_handshake_full() API.
> > 
> > With the new API, we can re-setup the TLS handshake GSource by calling
> > it again with the correct gcontext.  Any call to the function will clean
> > up existing GSource tasks, and re-setup using the new gcontext.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  chardev/char-socket.c    | 30 +++++++++++++---
> >  include/io/channel-tls.h | 22 +++++++++++-
> >  io/channel-tls.c         | 91 ++++++++++++++++++++++++++++++++++++++++--------
> >  3 files changed, 123 insertions(+), 20 deletions(-)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 164a64ff34..406d33c04f 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -72,6 +72,9 @@ typedef struct {
> >  
> >  static gboolean socket_reconnect_timeout(gpointer opaque);
> >  static void tcp_chr_telnet_init(Chardev *chr);
> > +static void tcp_chr_tls_handshake_setup(Chardev *chr,
> > +                                        QIOChannelTLS *tioc,
> > +                                        GMainContext *context);
> >  
> >  static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
> >  {
> > @@ -570,6 +573,7 @@ static void tcp_chr_telnet_destroy(SocketChardev *s)
> >  static void tcp_chr_update_read_handler(Chardev *chr)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> > +    QIOChannelTLS *tioc;
> >  
> >      if (s->listener) {
> >          /*
> > @@ -589,6 +593,17 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> >          qio_task_context_set(s->thread_task, chr->gcontext);
> >      }
> >  
> > +    tioc = (QIOChannelTLS *)object_dynamic_cast(OBJECT(s->ioc),
> > +                                                TYPE_QIO_CHANNEL_TLS);
> > +    if (tioc) {
> > +        /*
> > +         * TLS session enabled; reconfigure things up.  Note that, if
> > +         * there is existing handshake task, it'll be cleaned up first
> > +         * in QIO code.
> > +         */
> > +        tcp_chr_tls_handshake_setup(chr, tioc, chr->gcontext);
> > +    }
> 
> This is crazy - we should not be looking at specific implementations of
> the channel. If the TLS object needs to use a specific GMainContext we
> should make sure that is done right from the start and not try to change
> the GMainContext on the fly.

I'm not sure whether I can do it since current code has already let
the chardev frontends depend on the backends, so we cannot simply let
it be reverted (setup context basically means we need to have the
frontend be inited before backends since the context is now
frontend-specific).

However I'm thinking maybe I can postpone some of the chardev
initialization process after everything has been setup.  Then it'll
look like:

- init chardev backends, phase 1 (e.g., only create chardevs but
  postpone open)
- init chardev frontends (e.g., monitors)
- init chardev backends, phase 2 (e.g., do the real socket open work)

Actually I already spotted an existing user of it
(muxes_realize_notify).  Maybe I can do similar thing to postpone some
of the socket chardev operations after machine init finished.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts

[Peter Xu]

On Wed, Feb 28, 2018 at 01:20:24PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 09:05:26PM +0800, Peter Xu wrote:
> > On Wed, Feb 28, 2018 at 09:23:56AM +0000, Daniel P. Berrangé wrote:
> > > On Wed, Feb 28, 2018 at 01:06:32PM +0800, Peter Xu wrote:
> > > > This is the part of work to allow the QIOTask to use a different
> > > > gcontext rather than the default main gcontext, by providing
> > > > qio_task_context_set() API.
> > > > 
> > > > We have done some work before on doing similar things to add non-default
> > > > gcontext support.  The general idea is that we delete the old GSource
> > > > from the main context, then re-add a new one to the new context when
> > > > context changed to a non-default one.  However this trick won't work
> > > > easily for threaded QIOTasks since we can't easily stop a real thread
> > > > and re-setup the whole thing from the very beginning.
> > > 
> > > I think this entire usage pattern is really broken. We should not
> > > provide a way to change the GMainContext on an existing task. We
> > > should always just set the correct GMainContxt right from the start.
> > > This will avoid much of the complexity you're introducing into this
> > > patch series, and avoid having to expose GTasks to the callers of
> > > the async methods at all.
> > 
> > Yeah I agree with you that the threaded QIO patches are complicated.
> > Then how about I introduce:
> > 
> > - qio_task_thread_new(): to create QIO task and its thread (but not
> >                          running)
> > - qio_task_thread_run(): to run the thread
> > 
> > Then I can setup the context correctly between the two in some way.
> > 
> > After that, qio_task_run_in_thread() will be two calls of above two
> > APIs.
> 
> I don't see why it is not enough to just pass the right GMainContext
> into qio_task_run_in_thread() immediately. There should not be any
> need to split this into two parts. ALl that's needed here is for the
> completion callback to rnu in the right context, which just means
> passing the GMainContext from qio_task_run_in_thread, through to
> qio_task_thread_worker via QIOTaskThreadData. Changing the GMainContext
> after the thread is already created/running is not something we should
> try todo.

I dropped patches 9-14 in version two and rewrote as you suggested, by
introducing a chardev machine done notifier.  Please have a looks.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 04/14] migration: let incoming side use thread context

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Feb 28, 2018 at 05:43:50PM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > The old incoming migration is running in main thread and default
> > > gcontext.  With the new qio_channel_add_watch_full() we can now let it
> > > run in the thread's own gcontext (if there is one).
> > > 
> > > Currently this patch does nothing alone.  But when any of the incoming
> > > migration is run in another iothread (e.g., the upcoming migrate-recover
> > > command), this patch will bind the incoming logic to the iothread
> > > instead of the main thread (which may already get page faulted and
> > > hanged).
> > 
> > Does this make any difference to the Postcopy listener thread, which
> > takes over reading from the main thread once in postcopy mode?
> > (See savevm.c:postcopy_ram_listen_thread).
> 
> It should not.  It should only affect when use sends a
> "migrate-recover" with "run-oob=true".  The rest should be the same as
> before.  And since the postcopy ram load thread is a standalone thread
> with its own initial thread stack (so it's not really in a gmainloop),
> I can hardly tell how that can be affected since it'll always use its
> own thread stack.

OK, I think that's the bit I was worrying about; just since it was
another thread that ended up reading from the fd originally handled
by the incoming side main thread.

Dave

> Or, have I missed anything?
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK