[RFC,17/17] Simplify gdbserver's serial event handling

Message ID 20190224165153.5062-18-tom@tromey.com
State New
Headers show
Series
  • Merge event loop implementations
Related show

Commit Message

Tom Tromey Feb. 24, 2019, 4:51 p.m.
Currently, when gdbserver handles a serial event, it also calls
'reschedule' to install a timer that is then used to process any data
that remains after the previous packet was processed.

It seemed better to me to simply have the file descriptor callback
loop, processing packets until complete.

2019-02-24  Tom Tromey  <tom@tromey.com>

	* server.h (handle_serial_event): Update.
	* server.c (handle_serial_event): Return int.  Remove parameters.
	* remote-utils.c (readchar_callback): Remove.
	(handle_accept_event, remote_open): Use handle_all_serial_events.
	(readchar): Don't call reschedule.
	(reset_readchar): Update.
	(process_remaining, reschedule): Remove.
	(handle_all_serial_events): New function.
---
 gdb/gdbserver/ChangeLog      | 11 ++++++++
 gdb/gdbserver/remote-utils.c | 53 +++++++++++++-----------------------
 gdb/gdbserver/server.c       | 10 +++----
 gdb/gdbserver/server.h       |  2 +-
 4 files changed, 35 insertions(+), 41 deletions(-)

-- 
2.17.2

Comments

Pedro Alves Sept. 26, 2019, 5:36 p.m. | #1
On 2/24/19 4:51 PM, Tom Tromey wrote:
> Currently, when gdbserver handles a serial event, it also calls

> 'reschedule' to install a timer that is then used to process any data

> that remains after the previous packet was processed.

> 

> It seemed better to me to simply have the file descriptor callback

> loop, processing packets until complete.


I would rather not do this change, because it potentially starves other
event sources.  I've had to fix starvation issues like that in several
places in gdb throughout the years -- see url I pointed that justifying
need for async serial events, random_pending_event_thread in
infrun.c, etc.

Thanks,
Pedro Alves
Tom Tromey Oct. 4, 2019, 10:08 p.m. | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> On 2/24/19 4:51 PM, Tom Tromey wrote:
>> Currently, when gdbserver handles a serial event, it also calls

>> 'reschedule' to install a timer that is then used to process any data

>> that remains after the previous packet was processed.

>> 

>> It seemed better to me to simply have the file descriptor callback

>> loop, processing packets until complete.


Pedro> I would rather not do this change, because it potentially starves other
Pedro> event sources.  I've had to fix starvation issues like that in several
Pedro> places in gdb throughout the years -- see url I pointed that justifying
Pedro> need for async serial events, random_pending_event_thread in
Pedro> infrun.c, etc.

I have dropped it.

Tom

Patch

diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index b1e4e869b69..7b41090efcc 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -83,13 +83,9 @@  enum {
   NOT_SCHEDULED = -1
 };
 
-/* Status of the readchar callback.
-   Either NOT_SCHEDULED or the callback id.  */
-static int readchar_callback = NOT_SCHEDULED;
-
 static int readchar (void);
 static void reset_readchar (void);
-static void reschedule (void);
+static void handle_all_serial_events (int err, gdb_client_data client_data);
 
 /* A cache entry for a successfully looked-up symbol.  */
 struct sym_cache
@@ -204,7 +200,7 @@  handle_accept_event (int err, gdb_client_data client_data)
   enable_async_notification (remote_desc);
 
   /* Register the event loop handler.  */
-  add_file_handler (remote_desc, handle_serial_event, NULL);
+  add_file_handler (remote_desc, handle_all_serial_events, NULL);
 
   /* We have a new GDB connection now.  If we were disconnected
      tracing, there's a window where the target could report a stop
@@ -341,7 +337,7 @@  remote_open (const char *name)
       enable_async_notification (remote_desc);
 
       /* Register the event loop handler.  */
-      add_file_handler (remote_desc, handle_serial_event, NULL);
+      add_file_handler (remote_desc, handle_all_serial_events, NULL);
     }
 #ifndef USE_WIN32API
   else if (port_str == NULL)
@@ -382,7 +378,7 @@  remote_open (const char *name)
       enable_async_notification (remote_desc);
 
       /* Register the event loop handler.  */
-      add_file_handler (remote_desc, handle_serial_event, NULL);
+      add_file_handler (remote_desc, handle_all_serial_events, NULL);
     }
 #endif /* USE_WIN32API */
   else
@@ -879,9 +875,9 @@  initialize_async_io (void)
 #endif
 }
 
-/* Internal buffer used by readchar.
-   These are global to readchar because reschedule_remote needs to be
-   able to tell whether the buffer is empty.  */
+/* Internal buffer used by readchar.  These are global to readchar
+   because handle_all_serial_events needs to be able to tell whether
+   the buffer is empty.  */
 
 static unsigned char readchar_buf[BUFSIZ];
 static int readchar_bufcnt = 0;
@@ -916,7 +912,6 @@  readchar (void)
 
   readchar_bufcnt--;
   ch = *readchar_bufp++;
-  reschedule ();
   return ch;
 }
 
@@ -926,33 +921,23 @@  static void
 reset_readchar (void)
 {
   readchar_bufcnt = 0;
-  if (readchar_callback != NOT_SCHEDULED)
-    {
-      delete_timer (readchar_callback);
-      readchar_callback = NOT_SCHEDULED;
-    }
 }
 
-/* Process remaining data in readchar_buf.  */
+/* Loop, calling handle_serial_event, until there is no more data
+   available.  */
 
 static void
-process_remaining (void *context)
+handle_all_serial_events (int err, gdb_client_data client_data)
 {
-  /* This is a one-shot event.  */
-  readchar_callback = NOT_SCHEDULED;
-
-  if (readchar_bufcnt > 0)
-    handle_serial_event (0, NULL);
-}
-
-/* If there is still data in the buffer, queue another event to process it,
-   we can't sleep in select yet.  */
-
-static void
-reschedule (void)
-{
-  if (readchar_bufcnt > 0 && readchar_callback == NOT_SCHEDULED)
-    readchar_callback = create_timer (0, process_remaining, NULL);
+  do
+    {
+      if (handle_serial_event () < 0)
+	{
+	  stop_event_loop ();
+	  break;
+	}
+    }
+  while (readchar_bufcnt > 0);
 }
 
 /* Read a packet from the remote machine, with error checking,
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 42147c1d836..3732d53cfbc 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -4389,22 +4389,20 @@  process_serial_event (void)
 
 /* Event-loop callback for serial events.  */
 
-void
-handle_serial_event (int err, gdb_client_data client_data)
+int
+handle_serial_event ()
 {
   if (debug_threads)
     debug_printf ("handling possible serial event\n");
 
   /* Really handle it.  */
   if (process_serial_event () < 0)
-    {
-      stop_event_loop ();
-      return;
-    }
+    return -1;
 
   /* Be sure to not change the selected thread behind GDB's back.
      Important in the non-stop mode asynchronous protocol.  */
   set_desired_thread ();
+  return 0;
 }
 
 /* Push a stop notification on the notification queue.  */
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index e6a8dcc9bf1..9ffc9e0c958 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -83,7 +83,7 @@  extern int non_stop;
 /* Functions from server.c.  */
 extern void handle_v_requests (char *own_buf, int packet_len,
 			       int *new_packet_len);
-extern void handle_serial_event (int err, gdb_client_data client_data);
+extern int handle_serial_event ();
 extern void handle_target_event (int err, gdb_client_data client_data);
 
 /* Get rid of the currently pending stop replies that match PTID.  */