[Qemu-devel] [PATCH v4 0/3] chardev: convert leftover glib APIs to use dedicate gcontext

[Qemu-devel] [PATCH v4 0/3] chardev: convert leftover glib APIs to use dedicate gcontext

[Peter Xu]

v4:
- replace all timers from gsource tag IDs into gsource objects,
  otherwise g_source_remove() on that tag ID can errornously remove an
  gsource from default main context rather than the timer itself.

v3:
- rename function to qemu_chr_timeout_add_ms() [Stefan]
- add comment on return code [Stefan]
- add comment in commit message on why change GSource name [Stefan]

v2:
- add r-bs
- fix patch 3 on some s->ms conversion [Marc-André]

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_ms()

 chardev/char-fe.c      |  2 +-
 chardev/char-pty.c     | 64 +++++++++++++++++++++++++-------------------------
 chardev/char-socket.c  | 28 ++++++++++++++--------
 chardev/char.c         | 18 ++++++++++++++
 hw/char/terminal3270.c | 28 ++++++++++++----------
 include/chardev/char.h |  3 +++
 6 files changed, 88 insertions(+), 55 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v4 1/3] chardev: use backend chr context when watch for fe

[Peter Xu]

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 <stefanha@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-fe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index ee6d596100..c611b3fa3e 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -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

[Qemu-devel] [PATCH v4 2/3] chardev: let g_idle_add() be with chardev gcontext

[Peter Xu]

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 <peterx@redhat.com>
---
 chardev/char-pty.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 761ae6dec1..8248e36ab9 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -43,7 +43,7 @@ typedef struct {
     /* Protected by the Chardev chr_write_lock.  */
     int connected;
     guint timer_tag;
-    guint open_tag;
+    GSource *open_source;
 } PtyChardev;
 
 #define PTY_CHARDEV(obj) OBJECT_CHECK(PtyChardev, (obj), TYPE_CHARDEV_PTY)
@@ -58,7 +58,7 @@ static gboolean pty_chr_timer(gpointer opaque)
 
     qemu_mutex_lock(&chr->chr_write_lock);
     s->timer_tag = 0;
-    s->open_tag = 0;
+    s->open_source = NULL;
     if (!s->connected) {
         /* Next poll ... */
         pty_chr_update_read_handler_locked(chr);
@@ -183,7 +183,7 @@ static gboolean qemu_chr_be_generic_open_func(gpointer opaque)
     Chardev *chr = CHARDEV(opaque);
     PtyChardev *s = PTY_CHARDEV(opaque);
 
-    s->open_tag = 0;
+    s->open_source = NULL;
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
     return FALSE;
 }
@@ -194,9 +194,10 @@ static void pty_chr_state(Chardev *chr, int connected)
     PtyChardev *s = PTY_CHARDEV(chr);
 
     if (!connected) {
-        if (s->open_tag) {
-            g_source_remove(s->open_tag);
-            s->open_tag = 0;
+        if (s->open_source) {
+            g_source_destroy(s->open_source);
+            g_source_unref(s->open_source);
+            s->open_source = NULL;
         }
         remove_fd_in_watch(chr);
         s->connected = 0;
@@ -210,9 +211,13 @@ static void pty_chr_state(Chardev *chr, int connected)
             s->timer_tag = 0;
         }
         if (!s->connected) {
-            g_assert(s->open_tag == 0);
+            g_assert(s->open_source == NULL);
+            s->open_source = g_idle_source_new();
             s->connected = 1;
-            s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
+            g_source_set_callback(s->open_source,
+                                  qemu_chr_be_generic_open_func,
+                                  chr, NULL);
+            g_source_attach(s->open_source, chr->gcontext);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
-- 
2.14.3

[Qemu-devel] [PATCH v4 3/3] chardev: introduce qemu_chr_timeout_add_ms()

[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_ms() 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.

Also, convert all the timers from GSource tags into GSource pointers.
Gsource tag IDs and g_source_remove()s can only work with default
gcontext, while now these GSources can logically be attached to other
contexts.  So let's use explicit g_source_destroy() plus another
g_source_unref() to remove a timer.

Note: when in the timer handler, we don't need the g_source_destroy()
any more since that'll be done automatically if the timer handler
returns false (and that's what all the current handlers do).

Yet another note: in pty_chr_rearm_timer() we take special care for
ms=1000.  This patch merged the two cases into one.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-pty.c     | 43 +++++++++++++++++++------------------------
 chardev/char-socket.c  | 28 ++++++++++++++++++----------
 chardev/char.c         | 18 ++++++++++++++++++
 hw/char/terminal3270.c | 28 ++++++++++++++++------------
 include/chardev/char.h |  3 +++
 5 files changed, 74 insertions(+), 46 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 8248e36ab9..89315e6807 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -42,7 +42,7 @@ typedef struct {
 
     /* Protected by the Chardev chr_write_lock.  */
     int connected;
-    guint timer_tag;
+    GSource *timer_src;
     GSource *open_source;
 } PtyChardev;
 
@@ -57,7 +57,8 @@ static gboolean pty_chr_timer(gpointer opaque)
     PtyChardev *s = PTY_CHARDEV(opaque);
 
     qemu_mutex_lock(&chr->chr_write_lock);
-    s->timer_tag = 0;
+    s->timer_src = NULL;
+    g_source_unref(s->open_source);
     s->open_source = NULL;
     if (!s->connected) {
         /* Next poll ... */
@@ -67,25 +68,25 @@ static gboolean pty_chr_timer(gpointer opaque)
     return FALSE;
 }
 
+static void pty_chr_timer_cancel(PtyChardev *s)
+{
+    if (s->timer_src) {
+        g_source_destroy(s->timer_src);
+        g_source_unref(s->timer_src);
+        s->timer_src = NULL;
+    }
+}
+
 /* Called with chr_write_lock held.  */
 static void pty_chr_rearm_timer(Chardev *chr, int ms)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
     char *name;
 
-    if (s->timer_tag) {
-        g_source_remove(s->timer_tag);
-        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);
-    }
-    g_source_set_name_by_id(s->timer_tag, name);
+    pty_chr_timer_cancel(s);
+    name = g_strdup_printf("pty-timer-%s", chr->label);
+    s->timer_src = qemu_chr_timeout_add_ms(chr, ms, pty_chr_timer, chr);
+    g_source_set_name(s->timer_src, name);
     g_free(name);
 }
 
@@ -206,10 +207,7 @@ static void pty_chr_state(Chardev *chr, int connected)
          * the virtual device linked to our pty. */
         pty_chr_rearm_timer(chr, 1000);
     } else {
-        if (s->timer_tag) {
-            g_source_remove(s->timer_tag);
-            s->timer_tag = 0;
-        }
+        pty_chr_timer_cancel(s);
         if (!s->connected) {
             g_assert(s->open_source == NULL);
             s->open_source = g_idle_source_new();
@@ -236,10 +234,7 @@ static void char_pty_finalize(Object *obj)
     qemu_mutex_lock(&chr->chr_write_lock);
     pty_chr_state(chr, 0);
     object_unref(OBJECT(s->ioc));
-    if (s->timer_tag) {
-        g_source_remove(s->timer_tag);
-        s->timer_tag = 0;
-    }
+    pty_chr_timer_cancel(s);
     qemu_mutex_unlock(&chr->chr_write_lock);
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
@@ -272,7 +267,7 @@ static void char_pty_open(Chardev *chr,
     name = g_strdup_printf("chardev-pty-%s", chr->label);
     qio_channel_set_name(QIO_CHANNEL(s->ioc), name);
     g_free(name);
-    s->timer_tag = 0;
+    s->timer_src = NULL;
     *be_opened = false;
 }
 
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 630a7f2995..77cdf487eb 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -57,7 +57,7 @@ typedef struct {
     bool is_telnet;
     bool is_tn3270;
 
-    guint reconnect_timer;
+    GSource *reconnect_timer;
     int64_t reconnect_time;
     bool connect_err_reported;
 } SocketChardev;
@@ -67,16 +67,27 @@ typedef struct {
 
 static gboolean socket_reconnect_timeout(gpointer opaque);
 
+static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
+{
+    if (s->reconnect_timer) {
+        g_source_destroy(s->reconnect_timer);
+        g_source_unref(s->reconnect_timer);
+        s->reconnect_timer = NULL;
+    }
+}
+
 static void qemu_chr_socket_restart_timer(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
     char *name;
 
     assert(s->connected == 0);
-    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
-                                               socket_reconnect_timeout, chr);
     name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
-    g_source_set_name_by_id(s->reconnect_timer, name);
+    s->reconnect_timer = qemu_chr_timeout_add_ms(chr,
+                                                 s->reconnect_time * 1000,
+                                                 socket_reconnect_timeout,
+                                                 chr);
+    g_source_set_name(s->reconnect_timer, name);
     g_free(name);
 }
 
@@ -781,11 +792,7 @@ static void char_socket_finalize(Object *obj)
     SocketChardev *s = SOCKET_CHARDEV(obj);
 
     tcp_chr_free_connection(chr);
-
-    if (s->reconnect_timer) {
-        g_source_remove(s->reconnect_timer);
-        s->reconnect_timer = 0;
-    }
+    tcp_chr_reconn_timer_cancel(s);
     qapi_free_SocketAddress(s->addr);
     if (s->listener) {
         qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
@@ -824,7 +831,8 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
     SocketChardev *s = SOCKET_CHARDEV(opaque);
     QIOChannelSocket *sioc;
 
-    s->reconnect_timer = 0;
+    g_source_unref(s->reconnect_timer);
+    s->reconnect_timer = NULL;
 
     if (chr->be_open) {
         return false;
diff --git a/chardev/char.c b/chardev/char.c
index 8c3765ee99..3e14de1920 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1084,6 +1084,24 @@ 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), return
+ * the GSource object created. 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.
+ */
+GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms,
+                                 GSourceFunc func, void *private)
+{
+    GSource *source = g_timeout_source_new(ms);
+
+    assert(func);
+    g_source_set_callback(source, func, private, NULL);
+    g_source_attach(source, chr->gcontext);
+
+    return source;
+}
+
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index a109ce5987..e9c45e55b1 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -31,7 +31,7 @@ typedef struct Terminal3270 {
     uint8_t outv[OUTPUT_BUFFER_SIZE];
     int in_len;
     bool handshake_done;
-    guint timer_tag;
+    GSource *timer_src;
 } Terminal3270;
 
 #define TYPE_TERMINAL_3270 "x-terminal3270"
@@ -45,6 +45,15 @@ static int terminal_can_read(void *opaque)
     return INPUT_BUFFER_SIZE - t->in_len;
 }
 
+static void terminal_timer_cancel(Terminal3270 *t)
+{
+    if (t->timer_src) {
+        g_source_destroy(t->timer_src);
+        g_source_unref(t->timer_src);
+        t->timer_src = NULL;
+    }
+}
+
 /*
  * Protocol handshake done,
  * signal guest by an unsolicited DE irq.
@@ -90,12 +99,9 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
 
     assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
 
-    if (t->timer_tag) {
-        g_source_remove(t->timer_tag);
-        t->timer_tag = 0;
-    }
-    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
-
+    terminal_timer_cancel(t);
+    t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
+                                           send_timing_mark_cb, t);
     memcpy(&t->inv[t->in_len], buf, size);
     t->in_len += size;
     if (t->in_len < 2) {
@@ -145,10 +151,7 @@ static void chr_event(void *opaque, int event)
     /* Ensure the initial status correct, always reset them. */
     t->in_len = 0;
     t->handshake_done = false;
-    if (t->timer_tag) {
-        g_source_remove(t->timer_tag);
-        t->timer_tag = 0;
-    }
+    terminal_timer_cancel(t);
 
     switch (event) {
     case CHR_EVENT_OPENED:
@@ -157,7 +160,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_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
+                                               send_timing_mark_cb, t);
         break;
     case CHR_EVENT_CLOSED:
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 778d610295..d8941fcbb1 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -256,6 +256,9 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
 
 extern int term_escape_char;
 
+GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms,
+                                 GSourceFunc func, void *private);
+
 /* console.c */
 void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v4 0/3] chardev: convert leftover glib APIs to use dedicate gcontext

[Paolo Bonzini]

On 04/01/2018 15:18, Peter Xu wrote:
> v4:
> - replace all timers from gsource tag IDs into gsource objects,
>   otherwise g_source_remove() on that tag ID can errornously remove an
>   gsource from default main context rather than the timer itself.
> 
> v3:
> - rename function to qemu_chr_timeout_add_ms() [Stefan]
> - add comment on return code [Stefan]
> - add comment in commit message on why change GSource name [Stefan]
> 
> v2:
> - add r-bs
> - fix patch 3 on some s->ms conversion [Marc-André]
> 
> 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_ms()
> 
>  chardev/char-fe.c      |  2 +-
>  chardev/char-pty.c     | 64 +++++++++++++++++++++++++-------------------------
>  chardev/char-socket.c  | 28 ++++++++++++++--------
>  chardev/char.c         | 18 ++++++++++++++
>  hw/char/terminal3270.c | 28 ++++++++++++----------
>  include/chardev/char.h |  3 +++
>  6 files changed, 88 insertions(+), 55 deletions(-)
> 

Queued, thanks.

Paolo