Re: [Qemu-devel] [PULL 08/51] chardev: introduce qemu_chr_timeout_add_ms()
On Wed, Jan 17, 2018 at 05:21:40PM +0100, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jan 16, 2018 at 3:16 PM, Paolo Bonzini wrote:
> > From: 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
> > Message-Id: <20180104141835.17987-4-pet...@redhat.com>
> > Signed-off-by: Paolo Bonzini
> > ---
> > 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 8248e36..89315e6 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);
>
> why that line ^ ? It adds criticals every second (for ex with -chardev
> pty,id=foo -device isa-serial,chardev=foo).
My fault. I must have had a wrong rebase somehow after switching to
GSource pointers while kept the compiling happy. I'll post a fix
soon. Sorry!
--
Peter Xu
Re: [Qemu-devel] [PULL 08/51] chardev: introduce qemu_chr_timeout_add_ms()
Hi
On Tue, Jan 16, 2018 at 3:16 PM, Paolo Bonzini wrote:
> From: 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
> Message-Id: <20180104141835.17987-4-pet...@redhat.com>
> Signed-off-by: Paolo Bonzini
> ---
> 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 8248e36..89315e6 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);
why that line ^ ? It adds criticals every second (for ex with -chardev
pty,id=foo -device isa-serial,chardev=foo).
--
Marc-André Lureau
Re: [Qemu-devel] [PULL 08/51] chardev: introduce qemu_chr_timeout_add_ms()
On 16/01/2018 15:43, Daniel P. Berrange 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_ms() is introduced. > FYI the point of using g_timeout_add_seconds() is that it allow the > glib event loop to be more efficient. It ensures that all timers > which second granularity are dispatched on the same iteration of > the main loop. IOW, if you have 10 timers registered with > g_timeout_add_seconds() the main loop wakes up once a second and > runs all 10 of them. If you have 10 timers registered with g_timeout_add > the main loop wakes up 10 times a second, at a different time for each > timer. Yes, that can be added back later. In our case, it may even hurt to synchronize all timeouts at the same time (if there are many of them) because the BQL can introduce jitter. But it is difficult to say without measuring. Paolo
Re: [Qemu-devel] [PULL 08/51] chardev: introduce qemu_chr_timeout_add_ms()
On Tue, Jan 16, 2018 at 03:16:50PM +0100, Paolo Bonzini wrote: > From: 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. FYI the point of using g_timeout_add_seconds() is that it allow the glib event loop to be more efficient. It ensures that all timers which second granularity are dispatched on the same iteration of the main loop. IOW, if you have 10 timers registered with g_timeout_add_seconds() the main loop wakes up once a second and runs all 10 of them. If you have 10 timers registered with g_timeout_add the main loop wakes up 10 times a second, at a different time for each timer. 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 :|
[Qemu-devel] [PULL 08/51] chardev: introduce qemu_chr_timeout_add_ms()
From: 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
Message-Id: <20180104141835.17987-4-pet...@redhat.com>
Signed-off-by: Paolo Bonzini
---
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 8248e36..89315e6 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 630a7f2..77cdf48 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_tim
