| Message ID | 20171228072945.9573-4-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: convert leftover glib APIs to use dedicate gcontext | expand |
Hi On Thu, Dec 28, 2017 at 8:29 AM, Peter Xu <peterx@redhat.com> wrote: > 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 <peterx@redhat.com> > --- > 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(-) > > diff --git a/chardev/char-pty.c b/chardev/char-pty.c > index dd17b1b823..cbd8ac5eb7 100644 > --- a/chardev/char-pty.c > +++ b/chardev/char-pty.c > @@ -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); > } > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index 630a7f2995..644a620599 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr) > char *name; > > 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. > + 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); > diff --git a/chardev/char.c b/chardev/char.c > index 8c3765ee99..a1de662fec 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -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()); > diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c > index a109ce5987..479e4554d6 100644 > --- a/hw/char/terminal3270.c > +++ b/hw/char/terminal3270.c > @@ -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 > + 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 > + 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..4970e46457 100644 > --- a/include/chardev/char.h > +++ b/include/chardev/char.h > @@ -258,5 +258,7 @@ extern int term_escape_char; > > /* 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. > #endif > -- > 2.14.3 > >
On Tue, Jan 02, 2018 at 05:10:01PM +0100, Marc-André Lureau wrote: > Hi > > On Thu, Dec 28, 2017 at 8:29 AM, Peter Xu <peterx@redhat.com> wrote: > > 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 <peterx@redhat.com> > > --- > > 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(-) > > > > diff --git a/chardev/char-pty.c b/chardev/char-pty.c > > index dd17b1b823..cbd8ac5eb7 100644 > > --- a/chardev/char-pty.c > > +++ b/chardev/char-pty.c > > @@ -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); > > } > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > > index 630a7f2995..644a620599 100644 > > --- a/chardev/char-socket.c > > +++ b/chardev/char-socket.c > > @@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr) > > char *name; > > > > 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. Yes. Will fix them all. > > > + 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); > > diff --git a/chardev/char.c b/chardev/char.c > > index 8c3765ee99..a1de662fec 100644 > > --- a/chardev/char.c > > +++ b/chardev/char.c > > @@ -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()); > > diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c > > index a109ce5987..479e4554d6 100644 > > --- a/hw/char/terminal3270.c > > +++ b/hw/char/terminal3270.c > > @@ -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 > > > + 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 > > > + 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..4970e46457 100644 > > --- a/include/chardev/char.h > > +++ b/include/chardev/char.h > > @@ -258,5 +258,7 @@ extern int term_escape_char; > > > > /* 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. Sure. Thanks for review.
diff --git a/chardev/char-pty.c b/chardev/char-pty.c index dd17b1b823..cbd8ac5eb7 100644 --- a/chardev/char-pty.c +++ b/chardev/char-pty.c @@ -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); } diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 630a7f2995..644a620599 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr) char *name; 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, + 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); diff --git a/chardev/char.c b/chardev/char.c index 8c3765ee99..a1de662fec 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -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()); diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c index a109ce5987..479e4554d6 100644 --- a/hw/char/terminal3270.c +++ b/hw/char/terminal3270.c @@ -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, + 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, + 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..4970e46457 100644 --- a/include/chardev/char.h +++ b/include/chardev/char.h @@ -258,5 +258,7 @@ extern int term_escape_char; /* 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); #endif
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 <peterx@redhat.com> --- 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(-)