[Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb

[Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb

[Peter Wu]

While waiting for a gdb response, or while sending an acknowledgement
there is not much to do, so just mark the socket as non-blocking to
avoid a busy loop while paused at gdb. This only affects the user-mode
emulation (qemu-arm -g 1234 ./a.out).

Note that this issue was reported before at
https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html.

While at it, close the gdb client fd on EOF or error while reading.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 gdbstub.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 0e431fd..b126bf5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -332,7 +332,7 @@ static int get_char(GDBState *s)
         if (ret < 0) {
             if (errno == ECONNRESET)
                 s->fd = -1;
-            if (errno != EINTR && errno != EAGAIN)
+            if (errno != EINTR)
                 return -1;
         } else if (ret == 0) {
             close(s->fd);
@@ -393,7 +393,7 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int len)
     while (len > 0) {
         ret = send(s->fd, buf, len, 0);
         if (ret < 0) {
-            if (errno != EINTR && errno != EAGAIN)
+            if (errno != EINTR)
                 return;
         } else {
             buf += ret;
@@ -1542,9 +1542,13 @@ gdb_handlesig(CPUState *cpu, int sig)
             for (i = 0; i < n; i++) {
                 gdb_read_byte(s, buf[i]);
             }
-        } else if (n == 0 || errno != EAGAIN) {
+        } else {
             /* XXX: Connection closed.  Should probably wait for another
                connection before continuing.  */
+            if (n == 0) {
+                close(s->fd);
+            }
+            s->fd = -1;
             return sig;
         }
     }
@@ -1599,8 +1603,6 @@ static void gdb_accept(void)
     gdb_has_xml = false;
 
     gdbserver_state = s;
-
-    fcntl(fd, F_SETFL, O_NONBLOCK);
 }
 
 static int gdbserver_open(int port)
-- 
2.8.0

Re: [Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb

[Peter Maydell]

On 3 May 2016 at 23:58, Peter Wu <peter@lekensteyn.nl> wrote:
> While waiting for a gdb response, or while sending an acknowledgement
> there is not much to do, so just mark the socket as non-blocking to
> avoid a busy loop while paused at gdb. This only affects the user-mode
> emulation (qemu-arm -g 1234 ./a.out).
>
> Note that this issue was reported before at
> https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html.
>
> While at it, close the gdb client fd on EOF or error while reading.

The commit message says "mark the socket as non-blocking"...

> @@ -1599,8 +1603,6 @@ static void gdb_accept(void)
>      gdb_has_xml = false;
>
>      gdbserver_state = s;
> -
> -    fcntl(fd, F_SETFL, O_NONBLOCK);
>  }

...but the code change is *removing* a call to mark the
socket as non-blocking. Which is correct?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb

[Peter Wu]

On Thu, May 05, 2016 at 04:37:40PM +0100, Peter Maydell wrote:
> On 3 May 2016 at 23:58, Peter Wu <peter@lekensteyn.nl> wrote:
> > While waiting for a gdb response, or while sending an acknowledgement
> > there is not much to do, so just mark the socket as non-blocking to
> > avoid a busy loop while paused at gdb. This only affects the user-mode
> > emulation (qemu-arm -g 1234 ./a.out).
> >
> > Note that this issue was reported before at
> > https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html.
> >
> > While at it, close the gdb client fd on EOF or error while reading.
> 
> The commit message says "mark the socket as non-blocking"...
> 
> > @@ -1599,8 +1603,6 @@ static void gdb_accept(void)
> >      gdb_has_xml = false;
> >
> >      gdbserver_state = s;
> > -
> > -    fcntl(fd, F_SETFL, O_NONBLOCK);
> >  }
> 
> ...but the code change is *removing* a call to mark the
> socket as non-blocking. Which is correct?
> 
> thanks
> -- PMM

The commit message is misleading, it should have been "so do not mark
the socket as non-blocking". If you were to apply this, please fix it up
:-)
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb

[Peter Maydell]

On 3 May 2016 at 23:58, Peter Wu <peter@lekensteyn.nl> wrote:
> While waiting for a gdb response, or while sending an acknowledgement
> there is not much to do, so just mark the socket as non-blocking to
> avoid a busy loop while paused at gdb. This only affects the user-mode
> emulation (qemu-arm -g 1234 ./a.out).
>
> Note that this issue was reported before at
> https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html.
>
> While at it, close the gdb client fd on EOF or error while reading.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  gdbstub.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 0e431fd..b126bf5 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -332,7 +332,7 @@ static int get_char(GDBState *s)
>          if (ret < 0) {
>              if (errno == ECONNRESET)
>                  s->fd = -1;
> -            if (errno != EINTR && errno != EAGAIN)
> +            if (errno != EINTR)
>                  return -1;
>          } else if (ret == 0) {
>              close(s->fd);
> @@ -393,7 +393,7 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int len)
>      while (len > 0) {
>          ret = send(s->fd, buf, len, 0);
>          if (ret < 0) {
> -            if (errno != EINTR && errno != EAGAIN)
> +            if (errno != EINTR)
>                  return;
>          } else {
>              buf += ret;
> @@ -1542,9 +1542,13 @@ gdb_handlesig(CPUState *cpu, int sig)
>              for (i = 0; i < n; i++) {
>                  gdb_read_byte(s, buf[i]);
>              }
> -        } else if (n == 0 || errno != EAGAIN) {
> +        } else {
>              /* XXX: Connection closed.  Should probably wait for another
>                 connection before continuing.  */
> +            if (n == 0) {
> +                close(s->fd);
> +            }
> +            s->fd = -1;
>              return sig;
>          }
>      }
> @@ -1599,8 +1603,6 @@ static void gdb_accept(void)
>      gdb_has_xml = false;
>
>      gdbserver_state = s;
> -
> -    fcntl(fd, F_SETFL, O_NONBLOCK);
>  }
>
>  static int gdbserver_open(int port)

It does look like all the places we use s->fd:
 * are only for user-mode
 * are currently coded as "loop round until we get something",
   so effectively blocking but doing it with a busy loop

I suspect the O_NONBLOCK here may be legacy from before the
system-emulation gdbstub was reworked to use a chr backend
(in system emulation there really are other things we need
to attend to besides gdb stub packets, like the monitor).

Other than the error in the commit message,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM