[Qemu-devel] [PATCH 0/2] changes related to monitor flow control

[Qemu-devel] [PATCH 0/2] changes related to monitor flow control

[Laszlo Ersek]

When the IO thread calls monitor_flush() repeatedly & quickly in
succession, outside of callback context, many redundant G_IO_OUT watches
are installed. (One such caller is the "info tlb" / tlb_info() HMP
command which produces a lot of monitor output.)

While this redundancy is no problem in itself, it can trigger -1/EINVAL
in poll() by growing "gpollfds" beyond limits. This is a persistent
condition, causing qemu to spin in the main loop.

Patch #2 corrects this.

My first stab at a fix was patch #1. Although in retrospect probably
unrelated to the main problem, I'm including it because it should
qualify as an improvement / cleanup in its own right.

See <https://bugzilla.redhat.com/show_bug.cgi?id=970047> for more
details.

Laszlo Ersek (2):
  char: io_channel_send: don't lose written bytes
  monitor: maintain at most one G_IO_OUT watch

 monitor.c   |   11 +++++++++--
 qemu-char.c |   41 +++++++++++++++++++----------------------
 2 files changed, 28 insertions(+), 24 deletions(-)

[Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes

[Laszlo Ersek]

The g_io_channel_write_chars() documentation states,

  bytes_written: The number of bytes written. This can be nonzero even if
                 the return value is not G_IO_STATUS_NORMAL. [...]

io_channel_send() could lose such bytes before.

Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to
constant false whenever it was reached. When that condition actually held,
it always led to -1 / EINVAL. This patch (almost) distinguishes
G_IO_STATUS_EOF only when no bytes have been written, and then treats it
as an error.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qemu-char.c |   41 +++++++++++++++++++----------------------
 1 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 800d6a6..c86ce4b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -720,35 +720,32 @@ static GIOChannel *io_channel_from_socket(int fd)
 
 static int io_channel_send(GIOChannel *fd, const void *buf, size_t len)
 {
-    GIOStatus status;
-    size_t offset;
+    size_t offset = 0;
+    GIOStatus status = G_IO_STATUS_NORMAL;
 
-    offset = 0;
-    while (offset < len) {
-        gsize bytes_written;
+    while (offset < len && status == G_IO_STATUS_NORMAL) {
+        gsize bytes_written = 0;
 
         status = g_io_channel_write_chars(fd, buf + offset, len - offset,
                                           &bytes_written, NULL);
-        if (status != G_IO_STATUS_NORMAL) {
-            if (status == G_IO_STATUS_AGAIN) {
-                /* If we've written any data, return a partial write. */
-                if (offset) {
-                    break;
-                }
-                errno = EAGAIN;
-            } else {
-                errno = EINVAL;
-            }
-
-            return -1;
-        } else if (status == G_IO_STATUS_EOF) {
-            break;
-        }
-
         offset += bytes_written;
     }
 
-    return offset;
+    if (offset > 0) {
+        return offset;
+    }
+    switch (status) {
+    case G_IO_STATUS_NORMAL:
+        g_assert(len == 0);
+        return 0;
+    case G_IO_STATUS_AGAIN:
+        errno = EAGAIN;
+        return -1;
+    default:
+        break;
+    }
+    errno = EINVAL;
+    return -1;
 }
 
 #ifndef _WIN32
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes

[Anthony Liguori]

Laszlo Ersek <lersek@redhat.com> writes:

> The g_io_channel_write_chars() documentation states,
>
>   bytes_written: The number of bytes written. This can be nonzero even if
>                  the return value is not G_IO_STATUS_NORMAL. [...]
>
> io_channel_send() could lose such bytes before.
>
> Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to
> constant false whenever it was reached. When that condition actually held,
> it always led to -1 / EINVAL. This patch (almost) distinguishes
> G_IO_STATUS_EOF only when no bytes have been written, and then treats it
> as an error.

Just for my own benefit, I always assume G_IO_STATUS_EOF cannot happen
if bytes_written > 0.  I see what you mean by the comment but do you
have any reason to believe this happens in practice?

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qemu-char.c |   41 +++++++++++++++++++----------------------
>  1 files changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 800d6a6..c86ce4b 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -720,35 +720,32 @@ static GIOChannel *io_channel_from_socket(int fd)
>  
>  static int io_channel_send(GIOChannel *fd, const void *buf, size_t len)
>  {
> -    GIOStatus status;
> -    size_t offset;
> +    size_t offset = 0;
> +    GIOStatus status = G_IO_STATUS_NORMAL;
>  
> -    offset = 0;
> -    while (offset < len) {
> -        gsize bytes_written;
> +    while (offset < len && status == G_IO_STATUS_NORMAL) {
> +        gsize bytes_written = 0;
>  
>          status = g_io_channel_write_chars(fd, buf + offset, len - offset,
>                                            &bytes_written, NULL);
> -        if (status != G_IO_STATUS_NORMAL) {
> -            if (status == G_IO_STATUS_AGAIN) {
> -                /* If we've written any data, return a partial write. */
> -                if (offset) {
> -                    break;
> -                }
> -                errno = EAGAIN;
> -            } else {
> -                errno = EINVAL;
> -            }
> -
> -            return -1;
> -        } else if (status == G_IO_STATUS_EOF) {
> -            break;
> -        }
> -
>          offset += bytes_written;
>      }
>  
> -    return offset;
> +    if (offset > 0) {
> +        return offset;
> +    }
> +    switch (status) {
> +    case G_IO_STATUS_NORMAL:
> +        g_assert(len == 0);
> +        return 0;
> +    case G_IO_STATUS_AGAIN:
> +        errno = EAGAIN;
> +        return -1;
> +    default:
> +        break;
> +    }
> +    errno = EINVAL;
> +    return -1;
>  }
>  
>  #ifndef _WIN32
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes

[Laszlo Ersek]

On 07/16/13 20:57, Anthony Liguori wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> The g_io_channel_write_chars() documentation states,
>>
>>   bytes_written: The number of bytes written. This can be nonzero even if
>>                  the return value is not G_IO_STATUS_NORMAL. [...]
>>
>> io_channel_send() could lose such bytes before.
>>
>> Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to
>> constant false whenever it was reached. When that condition actually held,
>> it always led to -1 / EINVAL. This patch (almost) distinguishes
>> G_IO_STATUS_EOF only when no bytes have been written, and then treats it
>> as an error.
> 
> Just for my own benefit, I always assume G_IO_STATUS_EOF cannot happen
> if bytes_written > 0.  I see what you mean by the comment but do you
> have any reason to believe this happens in practice?

In my opinion, G_IO_STATUS_EOF doesn't make any sense whatsoever for a
write operation (for count>0) if glib kept any resemblance to write(),
and should never happen in practice.

The awkward commit message only captures the fact that I didn't forget
about G_IO_STATUS_EOF, I considered it explicitly.

> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Thanks!

Laszlo

Re: [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes

[Anthony Liguori]

Laszlo Ersek <lersek@redhat.com> writes:

> On 07/16/13 20:57, Anthony Liguori wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> 
>>> The g_io_channel_write_chars() documentation states,
>>>
>>>   bytes_written: The number of bytes written. This can be nonzero even if
>>>                  the return value is not G_IO_STATUS_NORMAL. [...]
>>>
>>> io_channel_send() could lose such bytes before.
>>>
>>> Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to
>>> constant false whenever it was reached. When that condition actually held,
>>> it always led to -1 / EINVAL. This patch (almost) distinguishes
>>> G_IO_STATUS_EOF only when no bytes have been written, and then treats it
>>> as an error.
>> 
>> Just for my own benefit, I always assume G_IO_STATUS_EOF cannot happen
>> if bytes_written > 0.  I see what you mean by the comment but do you
>> have any reason to believe this happens in practice?
>
> In my opinion, G_IO_STATUS_EOF doesn't make any sense whatsoever for a
> write operation (for count>0) if glib kept any resemblance to write(),
> and should never happen in practice.

Okay, thanks!

I'll give other folks a chance to look at this series and then apply in
a day or so.

Regards,

Anthony Liguori

>
> The awkward commit message only captures the fact that I didn't forget
> about G_IO_STATUS_EOF, I considered it explicitly.
>
>> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>
> Thanks!
>
> Laszlo

[Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch

[Laszlo Ersek]

When monitor_flush() is invoked repeatedly outside the monitor_unblocked()
callback, for example from tlb_info() -> ... -> print_pte(), several
watches may be added for the same event.

This is no problem per se because the extra monitor_unblocked() callbacks
are harmless if mon->outbuf is empty, the watches will be removed
gradually. However a big number of watches can grow "gpollfds" without
limit in glib_pollfds_fill(), triggering a -1/EINVAL condition in
g_poll().

Keep at most one such watch, by following the pattern observable in eg.
commits c874ea97 and c3d6b96e. The change has no effect when
monitor_unblocked() calls monitor_flush() (when the watch can either be
removed or renewed 1-for-1), but non-callback contexts won't create an
additional watch when the monitor already has one.

Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=970047

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 monitor.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2ba7876..de24b2c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -189,6 +189,7 @@ struct Monitor {
     int suspend_cnt;
     bool skip_flush;
     QString *outbuf;
+    guint watch;
     ReadLineState *rs;
     MonitorControl *mc;
     CPUState *mon_cpu;
@@ -263,7 +264,10 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
 static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
                                   void *opaque)
 {
-    monitor_flush(opaque);
+    Monitor *mon = opaque;
+
+    mon->watch = 0;
+    monitor_flush(mon);
     return FALSE;
 }
 
@@ -294,7 +298,10 @@ void monitor_flush(Monitor *mon)
             QDECREF(mon->outbuf);
             mon->outbuf = tmp;
         }
-        qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon);
+        if (mon->watch == 0) {
+            mon->watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
+                                               monitor_unblocked, mon);
+        }
     }
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch

[Anthony Liguori]

Laszlo Ersek <lersek@redhat.com> writes:

> When monitor_flush() is invoked repeatedly outside the monitor_unblocked()
> callback, for example from tlb_info() -> ... -> print_pte(), several
> watches may be added for the same event.
>
> This is no problem per se because the extra monitor_unblocked() callbacks
> are harmless if mon->outbuf is empty, the watches will be removed
> gradually. However a big number of watches can grow "gpollfds" without
> limit in glib_pollfds_fill(), triggering a -1/EINVAL condition in
> g_poll().
>
> Keep at most one such watch, by following the pattern observable in eg.
> commits c874ea97 and c3d6b96e. The change has no effect when
> monitor_unblocked() calls monitor_flush() (when the watch can either be
> removed or renewed 1-for-1), but non-callback contexts won't create an
> additional watch when the monitor already has one.
>
> Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=970047
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  monitor.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 2ba7876..de24b2c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -189,6 +189,7 @@ struct Monitor {
>      int suspend_cnt;
>      bool skip_flush;
>      QString *outbuf;
> +    guint watch;
>      ReadLineState *rs;
>      MonitorControl *mc;
>      CPUState *mon_cpu;
> @@ -263,7 +264,10 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>  static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>                                    void *opaque)
>  {
> -    monitor_flush(opaque);
> +    Monitor *mon = opaque;
> +
> +    mon->watch = 0;
> +    monitor_flush(mon);
>      return FALSE;
>  }
>  
> @@ -294,7 +298,10 @@ void monitor_flush(Monitor *mon)
>              QDECREF(mon->outbuf);
>              mon->outbuf = tmp;
>          }
> -        qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon);
> +        if (mon->watch == 0) {
> +            mon->watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
> +                                               monitor_unblocked, mon);
> +        }
>      }
>  }
>  
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 0/2] changes related to monitor flow control

[Amit Shah]

On (Tue) 16 Jul 2013 [20:19:39], Laszlo Ersek wrote:
> When the IO thread calls monitor_flush() repeatedly & quickly in
> succession, outside of callback context, many redundant G_IO_OUT watches
> are installed. (One such caller is the "info tlb" / tlb_info() HMP
> command which produces a lot of monitor output.)
> 
> While this redundancy is no problem in itself, it can trigger -1/EINVAL
> in poll() by growing "gpollfds" beyond limits. This is a persistent
> condition, causing qemu to spin in the main loop.
> 
> Patch #2 corrects this.
> 
> My first stab at a fix was patch #1. Although in retrospect probably
> unrelated to the main problem, I'm including it because it should
> qualify as an improvement / cleanup in its own right.
> 
> See <https://bugzilla.redhat.com/show_bug.cgi?id=970047> for more
> details.

Reviewed-by: Amit Shah <amit.shah@redhat.com>

		Amit

Re: [Qemu-devel] [PATCH 0/2] changes related to monitor flow control

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori