[RFC,17/17] Simplify gdbserver's serial event handling
Commit Message
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(-)
Comments
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
>>>>> "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
@@ -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,
@@ -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. */
@@ -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. */