chardev: convert leftover glib APIs to use dedicate gcontext

In commit 6bbb6c0644 ("chardev: use per-dev context for io_add_watch_poll", 2017-09-22) all the chardev watches are converted to use per-chardev gcontext to support chardev to be run outside default main thread. However that's still missing one call from the frontend code. Touch that up.

Reviewed-by Stefan Hajnoczi <[email protected]>

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

---
 chardev/char-fe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

chardev/char-fe.c +1 -2

index ee6d596100..c611b3fa3e 100644
@@ -356,7 +356,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
     }
 
     g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
-    tag = g_source_attach(src, NULL);
+    tag = g_source_attach(src, s->gcontext);
     g_source_unref(src);
 
     return tag;
--

2.14.3

The idle task will be attached to main gcontext even if the chardev backend is running in another gcontext. Fix the only caller by extending the g_idle_add() logic into the more powerful g_source_attach(). It's basically g_idle_add_full() implementation, but with the chardev's gcontext passed in.

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

---
 chardev/char-pty.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

chardev/char-pty.c +6 -2

index 761ae6dec1..dd17b1b823 100644
@@ -210,9 +210,14 @@ static void pty_chr_state(Chardev *chr, int connected)
             s->timer_tag = 0;
         }
         if (!s->connected) {
+            GSource *source = g_idle_source_new();
+
             g_assert(s->open_tag == 0);
             s->connected = 1;
-            s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
+            g_source_set_callback(source, qemu_chr_be_generic_open_func,
+                                  chr, NULL);
+            s->open_tag = g_source_attach(source, chr->gcontext);
+            g_source_unref(source);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
--

2.14.3

It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function qemu_chr_timeout_add() is introduced.

One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.

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

---
 chardev/char-pty.c     |  9 ++-------
 chardev/char-socket.c  |  4 ++--
 chardev/char.c         | 20 ++++++++++++++++++++
 hw/char/terminal3270.c |  7 ++++---
 include/chardev/char.h |  2 ++
 5 files changed, 30 insertions(+), 12 deletions(-)

chardev/char-pty.c +2 -7

index dd17b1b823..cbd8ac5eb7 100644
@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
         s->timer_tag = 0;
     }
 
-    if (ms == 1000) {
-        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
-        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
-    } else {
-        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
-        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
-    }
+    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
+    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
     g_source_set_name_by_id(s->timer_tag, name);
     g_free(name);
 }

chardev/char-socket.c +0 -0

index 630a7f2995..644a620599 100644

chardev/char-socket.c +1 -0

+                                              socket_reconnect_timeout, chr);
     name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
     g_source_set_name_by_id(s->reconnect_timer, name);
     g_free(name);

chardev/char.c +20 -0

index 8c3765ee99..a1de662fec 100644
@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)
     qemu_chr_be_event(chr, CHR_EVENT_BREAK);
 }
 
+/*
+ * Add a timeout callback for the chardev (in milliseconds). Please
+ * use this to add timeout hook for chardev instead of g_timeout_add()
+ * and g_timeout_add_seconds(), to make sure the gcontext that the
+ * task bound to is correct.
+ */
+guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
+                           void *private)
+{
+    GSource *source = g_timeout_source_new(ms);
+    guint id;
+
+    assert(func);
+    g_source_set_callback(source, func, private, NULL);
+    id = g_source_attach(source, chr->gcontext);
+    g_source_unref(source);
+
+    return id;
+}
+
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());

hw/char/terminal3270.c +0 -0

index a109ce5987..479e4554d6 100644

hw/char/terminal3270.c +1 -0

+                                            send_timing_mark_cb, t);
         break;
     case CHR_EVENT_CLOSED:
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;

include/chardev/char.h +0 -0

index 778d610295..4970e46457 100644

include/chardev/char.h +0 -1

 #endif
--

2.14.3

Hi

Quoted 47 lines · Peter Xu

It's a replacement of g_timeout_add[_seconds]() for chardevs. Chardevs now can have dedicated gcontext, we should always bind chardev tasks onto those gcontext rather than the default main context. Since there are quite a few of g_timeout_add[_seconds]() callers, a new function qemu_chr_timeout_add() is introduced.

One thing to mention is that, terminal3270 is still always running on main gcontext. However let's convert that as well since it's still part of chardev codes and in case one day we'll miss that when we move it out of main gcontext too.

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

---
 chardev/char-pty.c     |  9 ++-------
 chardev/char-socket.c  |  4 ++--
 chardev/char.c         | 20 ++++++++++++++++++++
 hw/char/terminal3270.c |  7 ++++---
 include/chardev/char.h |  2 ++
 5 files changed, 30 insertions(+), 12 deletions(-)

chardev/char-pty.c +2 -7

index dd17b1b823..cbd8ac5eb7 100644
@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
         s->timer_tag = 0;
     }
 
-    if (ms == 1000) {
-        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
-        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
-    } else {
-        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
-        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
-    }
+    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
+    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
     g_source_set_name_by_id(s->timer_tag, name);
     g_free(name);
 }

chardev/char-socket.c +0 -0

index 630a7f2995..644a620599 100644
@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
     char *name;
 

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

Quoted 4 lines

chardev/char-socket.c +1 -2

     assert(s->connected == 0);
-    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
-                                               socket_reconnect_timeout, chr);
+    s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time,

reconnect_time is in second, you should * 1000.

Quoted 41 lines

chardev/char-socket.c +1 -0

+                                              socket_reconnect_timeout, chr);
     name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
     g_source_set_name_by_id(s->reconnect_timer, name);
     g_free(name);

chardev/char.c +20 -0

index 8c3765ee99..a1de662fec 100644
@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)
     qemu_chr_be_event(chr, CHR_EVENT_BREAK);
 }
 
+/*
+ * Add a timeout callback for the chardev (in milliseconds). Please
+ * use this to add timeout hook for chardev instead of g_timeout_add()
+ * and g_timeout_add_seconds(), to make sure the gcontext that the
+ * task bound to is correct.
+ */
+guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
+                           void *private)
+{
+    GSource *source = g_timeout_source_new(ms);
+    guint id;
+
+    assert(func);
+    g_source_set_callback(source, func, private, NULL);
+    id = g_source_attach(source, chr->gcontext);
+    g_source_unref(source);
+
+    return id;
+}
+
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());

hw/char/terminal3270.c +0 -0

index a109ce5987..479e4554d6 100644

hw/char/terminal3270.c static void terminal_read(void *opaque, const uint8_t *buf, int size) · line 97

Quoted 6 lines

hw/char/terminal3270.c +1 -2

@@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
         g_source_remove(t->timer_tag);
         t->timer_tag = 0;
     }
-    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
-
+    t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600,

same here

hw/char/terminal3270.c static void chr_event(void *opaque, int event) · line 160

Quoted 10 lines

hw/char/terminal3270.c +2 -1

+                                        send_timing_mark_cb, t);
     memcpy(&t->inv[t->in_len], buf, size);
     t->in_len += size;
     if (t->in_len < 2) {

@@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)
          * char-socket.c. Once qemu receives the terminal-type of the
          * client, mark handshake done and trigger everything rolling again.
          */
-        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
+        t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600,

and again

Quoted 10 lines

hw/char/terminal3270.c +1 -0

+                                            send_timing_mark_cb, t);
         break;
     case CHR_EVENT_CLOSED:
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;

include/chardev/char.h +0 -0

index 778d610295..4970e46457 100644
@@ -258,5 +258,7 @@ extern int term_escape_char;
 

include/chardev/char.h extern int term_escape_char; · line 263

Quoted 5 lines

include/chardev/char.h +2 -0

 /* console.c */
 void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp);
+guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
+                           void *private);
 

Please put the declaration before the /* console.c */ comment.

There were existing work that tried to allow chardev to be run in a dedicated gcontext rather than the default main context/thread. Basically that work passed in the correct gcontext during g_source_attach(). However, I found something missing along the way, that some legacy glib APIs are used by chardev code which take the main context as default:

g_timeout_add_seconds
g_timeout_add
g_idle_add

To fully allow the chardevs to be run in dedicated gcontext, we need to convert all these legacy APIs into g_source_attach() calls as well, with the correct gcontext passed in.

This series tries to clean the rest of things up.

I picked up patch 1 from monitor-oob series into this series (which is a missing of chardev frontend call fix for g_source_attach()), so that this series can be a complete fix.

Please review. Thanks,

Peter Xu (3):
chardev: use backend chr context when watch for fe
chardev: let g_idle_add() be with chardev gcontext
chardev: introduce qemu_chr_timeout_add() and use

 chardev/char-fe.c      |  2 +-
 chardev/char-pty.c     | 16 ++++++++--------
 chardev/char-socket.c  |  4 ++--
 chardev/char.c         | 20 ++++++++++++++++++++
 hw/char/terminal3270.c |  7 ++++---
 include/chardev/char.h |  2 ++
 6 files changed, 37 insertions(+), 14 deletions(-)

Quoted 28 lines · Peter Xu

There were existing work that tried to allow chardev to be run in a dedicated gcontext rather than the default main context/thread. Basically that work passed in the correct gcontext during g_source_attach(). However, I found something missing along the way, that some legacy glib APIs are used by chardev code which take the main context as default:

g_timeout_add_seconds
g_timeout_add
g_idle_add

To fully allow the chardevs to be run in dedicated gcontext, we need to convert all these legacy APIs into g_source_attach() calls as well, with the correct gcontext passed in.

This series tries to clean the rest of things up.

I picked up patch 1 from monitor-oob series into this series (which is a missing of chardev frontend call fix for g_source_attach()), so that this series can be a complete fix.

Please review. Thanks,

Peter Xu (3):
chardev: use backend chr context when watch for fe
chardev: let g_idle_add() be with chardev gcontext
chardev: introduce qemu_chr_timeout_add() and use

Quoted 8 lines

 chardev/char-fe.c      |  2 +-
 chardev/char-pty.c     | 16 ++++++++--------
 chardev/char-socket.c  |  4 ++--
 chardev/char.c         | 20 ++++++++++++++++++++
 hw/char/terminal3270.c |  7 ++++---
 include/chardev/char.h |  2 ++
 6 files changed, 37 insertions(+), 14 deletions(-)

All queued, thanks!