* [Qemu-devel] [PATCH 0/2] changes related to monitor flow control
@ 2013-07-16 18:19 Laszlo Ersek
2013-07-16 18:19 ` [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes Laszlo Ersek
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Laszlo Ersek @ 2013-07-16 18:19 UTC (permalink / raw)
To: Anthony Liguori, Luiz Capitulino, qemu-devel
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(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes
2013-07-16 18:19 [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Laszlo Ersek
@ 2013-07-16 18:19 ` Laszlo Ersek
2013-07-16 18:57 ` Anthony Liguori
2013-07-16 18:19 ` [Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2013-07-16 18:19 UTC (permalink / raw)
To: Anthony Liguori, Luiz Capitulino, qemu-devel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch
2013-07-16 18:19 [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Laszlo Ersek
2013-07-16 18:19 ` [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes Laszlo Ersek
@ 2013-07-16 18:19 ` Laszlo Ersek
2013-07-16 18:58 ` Anthony Liguori
2013-07-17 10:34 ` [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Amit Shah
2013-07-18 19:36 ` Anthony Liguori
3 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2013-07-16 18:19 UTC (permalink / raw)
To: Anthony Liguori, Luiz Capitulino, qemu-devel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes
2013-07-16 18:19 ` [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes Laszlo Ersek
@ 2013-07-16 18:57 ` Anthony Liguori
2013-07-16 19:26 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Anthony Liguori @ 2013-07-16 18:57 UTC (permalink / raw)
To: Laszlo Ersek, Luiz Capitulino, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch
2013-07-16 18:19 ` [Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch Laszlo Ersek
@ 2013-07-16 18:58 ` Anthony Liguori
0 siblings, 0 replies; 9+ messages in thread
From: Anthony Liguori @ 2013-07-16 18:58 UTC (permalink / raw)
To: Laszlo Ersek, Luiz Capitulino, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes
2013-07-16 18:57 ` Anthony Liguori
@ 2013-07-16 19:26 ` Laszlo Ersek
2013-07-16 19:51 ` Anthony Liguori
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2013-07-16 19:26 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes
2013-07-16 19:26 ` Laszlo Ersek
@ 2013-07-16 19:51 ` Anthony Liguori
0 siblings, 0 replies; 9+ messages in thread
From: Anthony Liguori @ 2013-07-16 19:51 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] changes related to monitor flow control
2013-07-16 18:19 [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Laszlo Ersek
2013-07-16 18:19 ` [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes Laszlo Ersek
2013-07-16 18:19 ` [Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch Laszlo Ersek
@ 2013-07-17 10:34 ` Amit Shah
2013-07-18 19:36 ` Anthony Liguori
3 siblings, 0 replies; 9+ messages in thread
From: Amit Shah @ 2013-07-17 10:34 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Anthony Liguori, qemu-devel, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] changes related to monitor flow control
2013-07-16 18:19 [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Laszlo Ersek
` (2 preceding siblings ...)
2013-07-17 10:34 ` [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Amit Shah
@ 2013-07-18 19:36 ` Anthony Liguori
3 siblings, 0 replies; 9+ messages in thread
From: Anthony Liguori @ 2013-07-18 19:36 UTC (permalink / raw)
To: Laszlo Ersek, Anthony Liguori, Luiz Capitulino, qemu-devel
Applied. Thanks.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-07-18 19:36 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-16 18:19 [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Laszlo Ersek
2013-07-16 18:19 ` [Qemu-devel] [PATCH 1/2] char: io_channel_send: don't lose written bytes Laszlo Ersek
2013-07-16 18:57 ` Anthony Liguori
2013-07-16 19:26 ` Laszlo Ersek
2013-07-16 19:51 ` Anthony Liguori
2013-07-16 18:19 ` [Qemu-devel] [PATCH 2/2] monitor: maintain at most one G_IO_OUT watch Laszlo Ersek
2013-07-16 18:58 ` Anthony Liguori
2013-07-17 10:34 ` [Qemu-devel] [PATCH 0/2] changes related to monitor flow control Amit Shah
2013-07-18 19:36 ` Anthony Liguori
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.