qio: general non-default GMainContext support

Need to free TCPChardevTelnetInit when session established.

Since at it, switch to use G_SOURCE_* macros.

Signed-off-by Peter Xu <[email protected]>

---
 chardev/char-socket.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

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 <[email protected]>

---
 io/task.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

io/task.c +2 -2

index 3ce556017c..1a0a1c7185 100644
@@ -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;
 }
 

Signature · 1 line

2.14.3

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 <[email protected]>

---
 include/io/channel.h | 31 ++++++++++++++++++++++++++++---
 io/channel.c         | 24 +++++++++++++++++++-----
 2 files changed, 47 insertions(+), 8 deletions(-)

include/io/channel.h +28 -3

index 3995e243a3..36af5e58ae 100644
@@ -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.

io/channel.c +19 -5

index ec4b86de7c..3e734cc9a5 100644
@@ -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;

Signature · 1 line

2.14.3

Quoted 4 lines · 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.

Quoted 2 lines

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.

Quoted 5 lines · Daniel P. Berrangé

Quoted 4 lines · 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.

Quoted 7 lines

Quoted 2 lines

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?

Quoted 14 lines · Peter Xu

Quoted 12 lines · Daniel P. Berrangé

Quoted 6 lines · 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.

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.

Quoted 5 lines

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.

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 <[email protected]>

CC Dr. David Alan Gilbert <[email protected]>

CC Laurent Vivier <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 migration/exec.c   | 11 ++++++-----
 migration/fd.c     | 11 ++++++-----
 migration/socket.c | 12 +++++++-----
 3 files changed, 19 insertions(+), 15 deletions(-)

migration/exec.c +6 -5

index 0bc5a427dd..f401fc005e 100644
@@ -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);
 }

migration/fd.c +6 -5

index cd06182d1e..9c593eb3ff 100644
@@ -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);
 }

migration/socket.c +7 -5

index e090097077..82c330083c 100644
@@ -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)

Signature · 1 line

2.14.3

Quoted 4 lines · 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).

Quoted 8 lines

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 */
  };
  ....

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 <[email protected]>

CC Dr. David Alan Gilbert <[email protected]>

CC Laurent Vivier <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 migration/exec.c   | 11 ++++++-----
 migration/fd.c     | 11 ++++++-----
 migration/socket.c | 12 +++++++-----
 3 files changed, 19 insertions(+), 15 deletions(-)

migration/exec.c +6 -5

index 0bc5a427dd..f401fc005e 100644
@@ -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);
 }

migration/fd.c +6 -5

index cd06182d1e..9c593eb3ff 100644
@@ -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);
 }

migration/socket.c +7 -5

index e090097077..82c330083c 100644
@@ -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)

Signature · 1 line

2.14.3

Quoted 9 lines · * Peter Xu ([email protected]) 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).

Quoted 11 lines · Dr. David Alan Gilbert

Quoted 9 lines · * Peter Xu ([email protected]) 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).

Quoted 3 lines

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?

Signature · 1 line

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 <[email protected]>

---
 chardev/char-socket.c     |  9 +++++++++
 include/io/net-listener.h | 12 ++++++++++++
 io/net-listener.c         |  7 +++++++
 3 files changed, 28 insertions(+)

chardev/char-socket.c +9 -0

index 43a2cc2c1c..8f0935cd15 100644
@@ -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;
     }

include/io/net-listener.h +1 -0

index 566be283b3..39dede9d6f 100644
@@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
                                SocketAddress *addr,
                                Error **errp);
 
+/**

Quoted 48 lines · 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 <[email protected]>

---
 chardev/char-socket.c     |  9 +++++++++
 include/io/net-listener.h | 12 ++++++++++++
 io/net-listener.c         |  7 +++++++
 3 files changed, 28 insertions(+)

chardev/char-socket.c +9 -0

index 43a2cc2c1c..8f0935cd15 100644
@@ -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;
     }

include/io/net-listener.h +0 -0

index 566be283b3..39dede9d6f 100644
@@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
                                SocketAddress *addr,
                                Error **errp);
 

include/io/net-listener.h int qio_net_listener_open_sync(QIONetListener *listener, · line 119

Quoted 11 lines

include/io/net-listener.h +11 -0

+/**
+ * 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

Quoted 49 lines · Daniel P. Berrangé

Quoted 48 lines · 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 <[email protected]>

---
 chardev/char-socket.c     |  9 +++++++++
 include/io/net-listener.h | 12 ++++++++++++
 io/net-listener.c         |  7 +++++++
 3 files changed, 28 insertions(+)

chardev/char-socket.c +9 -0

index 43a2cc2c1c..8f0935cd15 100644
@@ -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;
     }

include/io/net-listener.h +0 -0

index 566be283b3..39dede9d6f 100644
@@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
                                SocketAddress *addr,
                                Error **errp);
 

Quoted 15 lines

include/io/net-listener.h int qio_net_listener_open_sync(QIONetListener *listener, · line 119

Quoted 11 lines

include/io/net-listener.h +11 -0

+/**
+ * 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?

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

Signed-off-by Peter Xu <[email protected]>

---
 include/io/task.h |  3 +++
 io/task.c         | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

include/io/task.h +0 -0

index 9dbe3758d7..c6acd6489c 100644

io/task.c +22 -1

index 204c0be286..00d3a5096a 100644
@@ -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);
+    }
+}

Signature · 1 line

2.14.3

Quoted 17 lines · Peter Xu

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

Signed-off-by Peter Xu <[email protected]>

---
 include/io/task.h |  3 +++
 io/task.c         | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

include/io/task.h +0 -0

index 9dbe3758d7..c6acd6489c 100644
@@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
  */
 Object *qio_task_get_source(QIOTask *task);
 

include/io/task.h gpointer qio_task_get_result_pointer(QIOTask *task); · line 326

Quoted 2 lines

include/io/task.h +2 -0

+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.

Quoted 18 lines · Daniel P. Berrangé

Quoted 17 lines · Peter Xu

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

Signed-off-by Peter Xu <[email protected]>

---
 include/io/task.h |  3 +++
 io/task.c         | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

include/io/task.h +0 -0

index 9dbe3758d7..c6acd6489c 100644
@@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
  */
 Object *qio_task_get_source(QIOTask *task);
 

Quoted 5 lines

include/io/task.h gpointer qio_task_get_result_pointer(QIOTask *task); · line 326

Quoted 2 lines

include/io/task.h +2 -0

+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,

Quoted 19 lines · Peter Xu

Quoted 18 lines · Daniel P. Berrangé

Quoted 17 lines · Peter Xu

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

Signed-off-by Peter Xu <[email protected]>

---
 include/io/task.h |  3 +++
 io/task.c         | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

include/io/task.h +0 -0

index 9dbe3758d7..c6acd6489c 100644
@@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
  */
 Object *qio_task_get_source(QIOTask *task);
 

Quoted 8 lines

Quoted 5 lines

include/io/task.h gpointer qio_task_get_result_pointer(QIOTask *task); · line 326

Quoted 2 lines

include/io/task.h +2 -0

+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

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.

Quoted 3 lines · 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.

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.

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.

Quoted 10 lines · 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.

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.

Quoted 12 lines · Daniel P. Berrangé

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 23 lines · Peter Xu

Quoted 12 lines · Daniel P. Berrangé

Quoted 10 lines · 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.

Quoted 6 lines

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:

Quoted 8 lines

- 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.

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 <[email protected]>

---
 chardev/char-socket.c    | 30 +++++++++++++---
 include/io/channel-tls.h | 22 +++++++++++-
 io/channel-tls.c         | 91 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 123 insertions(+), 20 deletions(-)

chardev/char-socket.c +4 -0

index 164a64ff34..406d33c04f 100644
@@ -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);
     }
 

Quoted 40 lines · 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 <[email protected]>

---
 chardev/char-socket.c    | 30 +++++++++++++---
 include/io/channel-tls.h | 22 +++++++++++-
 io/channel-tls.c         | 91 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 123 insertions(+), 20 deletions(-)

chardev/char-socket.c +4 -0

index 164a64ff34..406d33c04f 100644
@@ -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);
     }
 

chardev/char-socket.c static void tcp_chr_update_read_handler(Chardev *chr) · line 605

Quoted 10 lines

chardev/char-socket.c +10 -0

+    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.