Re: [PATCH 0/2] Better handling of slow remote transfers

[Pedro Alves <palves at redhat dot com>]

On 08/19/2015 02:42 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> BTW, the transfers seem to be always interruptible for me, even without
>> Gary's patch, and even the slow ones.
> 
> Ok, I'm glad I'm not the only one :)
> 
>> From d426ce0a36830378a8ec2e2cbdd94d9fcb47b891 Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <palves@redhat.com>
>> Date: Tue, 18 Aug 2015 23:27:20 +0100
>> Subject: [PATCH 3/3] add readahead cache to gdb's vFile:pread
> 
> I tried this out on its own, and got similar hit/miss ratios, so it
> looks like a good addition.  Comments below.
> 
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index a839adf..8a739c8 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -10311,6 +10311,26 @@ remote_hostio_send_command (int command_bytes, int which_packet,
>>    return ret;
>>  }
>>  
>> +struct readahead_cache
>> +{
>> +  int fd;
>> +  gdb_byte *buf;
>> +  ULONGEST offset;
>> +  size_t bufsize;
>> +};
>> +
>> +static struct readahead_cache *readahead_cache;
> 
> Would this be better in struct remote_state (and maybe not allocated,
> just inlined in the main remote_state--it's only 16 or 32 bytes)?

Definitely.

(At first I made the cache invalidation free
the cache object (and set it NULL).  Then I when I saw
that the CPU usage went up, I made the invalidation just
clear fd to -1.  The CPU did not go down.  Then I lowered
back the max packet size and the CPU usage went back down,
but I kept the cache invalidation as it was.)

> 
>> @@ -10325,6 +10345,8 @@ remote_hostio_set_filesystem
>>    char arg[9];
>>    int ret;
>>  
>> +  readahead_cache_invalidate ();
>> +
>>    if (packet_support (PACKET_vFile_setfs) == PACKET_DISABLE)
>>      return 0;
>>  
> 
> This isn't necessary, file descriptors persist across setns calls.

OK.

> 
>> +      if (remote_debug)
>> +	fprintf_unfiltered (gdb_stdlog,
>> +			    "readahead cache hit %d\n",
>> +			    ++readahead_cache_hit_count);
> and
>> +  if (remote_debug)
>> +    fprintf_unfiltered (gdb_stdlog, "readahead cache miss %d\n",
>> +			++readahead_cache_miss_count);
> 
> These only update the counters when debug printing is switched on.
> Is this what you intended?

Hey, that was a quick and dirty patch.  :-)

Here's a cleaned up version.  WDYT?

---
>From d853f786962906127da99c0ac42745447a9bbd05 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 20 Aug 2015 14:15:48 +0100
Subject: [PATCH] Add readahead cache to gdb's vFile:pread

This patch almost halves the time it takes to "target remote + run to
main" on a higher-latency connection.

E.g., I've got a ping time of ~85ms to an x86-64 machine on the gcc
compile farm (almost 2000km away from me), and I'm behind a ~16Mbit
ADSL.  When I connect to a gdbserver debugging itself on that machine
and run to main, it takes almost 55 seconds:

 [palves@gcc76] $ ./gdbserver :9999 ./gdbserver
 [palves@home] $ ssh -L 9999:localhost:9999 gcc76.fsffrance.org
 [palves@home] time ./gdb -data-directory=data-directory -ex "tar rem :9999" -ex "b main" -ex "c" -ex "set confirm off" -ex "quit"

 Pristine gdb 7.10.50.20150820-cvs gets us:
 ...
 Remote debugging using :9999
 Reading symbols from target:/home/palves/gdb/build/gdb/gdbserver/gdbserver...done.
 Reading symbols from target:/lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
 0x00007ffff7ddd190 in ?? () from target:/lib64/ld-linux-x86-64.so.2
 Breakpoint 1 at 0x41200c: file ../../../src/gdb/gdbserver/server.c, line 3635.
 Continuing.

 Breakpoint 1, main (argc=1, argv=0x7fffffffe3d8) at ../../../src/gdb/gdbserver/server.c:3635
 3635    ../../../src/gdb/gdbserver/server.c: No such file or directory.
 /home/palves/gdb/build/gdb/gdbserver/gdbserver: No such file or directory.

 real    0m54.803s
 user    0m0.329s
 sys     0m0.064s

While the readahead cache added by this patch, it drops to:

 real    0m29.462s
 user    0m0.454s
 sys     0m0.054s

I added a few counters to show cache hit/miss, and got:

 readahead cache miss 142
 readahead cache hit 310

Tested on x86_64 Fedora 20.

gdb/ChangeLog:
2015-08-20  Pedro Alves  <palves@redhat.com>

	* remote.c (struct readahead_cache): New.
	(struct remote_state) <readahead_cache>: New field.
	(remote_open_1): Invalidate the cache.
	(readahead_cache_invalidate, readahead_cache_invalidate_fd): New
	functions.
	(remote_hostio_pwrite): Invalidate the readahead cache.
	(remote_hostio_pread): Rename to ...
	(remote_hostio_pread_vFile): ... this.
	(remote_hostio_pread_from_cache): New function.
	(remote_hostio_pread): Reimplement.
	(remote_hostio_close): Invalidate the readahead cache.
---
 gdb/remote.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 135 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 4e483fd..bfa5469 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -226,6 +226,8 @@ static int remote_can_run_breakpoint_commands (struct target_ops *self);
 
 static void remote_btrace_reset (void);
 
+static void readahead_cache_invalidate (void);
+
 /* For "remote".  */
 
 static struct cmd_list_element *remote_cmdlist;
@@ -262,6 +264,29 @@ typedef unsigned char threadref[OPAQUETHREADBYTES];
 
 #define MAXTHREADLISTRESULTS 32
 
+/* Data for the vFile:pread readahead cache.  */
+
+struct readahead_cache
+{
+  /* The file descriptor for the file that is being cached.  -1 if the
+     cache is invalid.  */
+  int fd;
+
+  /* The offset into the file that the cache buffer corresponds
+     to.  */
+  ULONGEST offset;
+
+  /* The buffer holding the cache contents.  */
+  gdb_byte *buf;
+  /* The buffer's size.  We try to read as much as fits into a packet
+     at a time.  */
+  size_t bufsize;
+
+  /* Cache hit and miss counters.  */
+  ULONGEST hit_count;
+  ULONGEST miss_count;
+};
+
 /* Description of the remote protocol state for the currently
    connected target.  This is per-target state, and independent of the
    selected architecture.  */
@@ -381,6 +406,14 @@ struct remote_state
      Initialized to -1 to indicate that no "vFile:setfs:" packet
      has yet been sent.  */
   int fs_pid;
+
+  /* A readahead cache for vFile:pread.  Often, reading a binary
+     involves a sequence of small reads.  E.g., when parsing an ELF
+     file.  A readahead cache helps mostly the case of remote
+     debugging on a connection with higher latency, due to the
+     request/reply nature of the RSP.  We only cache data for a single
+     file descriptor at a time.  */
+  struct readahead_cache readahead_cache;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -4498,6 +4531,8 @@ remote_open_1 (const char *name, int from_tty,
   rs->use_threadinfo_query = 1;
   rs->use_threadextra_query = 1;
 
+  readahead_cache_invalidate ();
+
   if (target_async_permitted)
     {
       /* With this target we start out by owning the terminal.  */
@@ -10329,6 +10364,27 @@ remote_hostio_send_command (int command_bytes, int which_packet,
   return ret;
 }
 
+/* Invalidate the readahead cache.  */
+
+static void
+readahead_cache_invalidate (void)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  rs->readahead_cache.fd = -1;
+}
+
+/* Invalidate the readahead cache if it is holding data for FD.  */
+
+static void
+readahead_cache_invalidate_fd (int fd)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  if (rs->readahead_cache.fd == fd)
+    rs->readahead_cache.fd = -1;
+}
+
 /* Set the filesystem remote_hostio functions that take FILENAME
    arguments will use.  Return 0 on success, or -1 if an error
    occurs (and set *REMOTE_ERRNO).  */
@@ -10407,6 +10463,8 @@ remote_hostio_pwrite (struct target_ops *self,
   int left = get_remote_packet_size ();
   int out_len;
 
+  readahead_cache_invalidate_fd (fd);
+
   remote_buffer_add_string (&p, &left, "vFile:pwrite:");
 
   remote_buffer_add_int (&p, &left, fd);
@@ -10422,12 +10480,13 @@ remote_hostio_pwrite (struct target_ops *self,
 				     remote_errno, NULL, NULL);
 }
 
-/* Implementation of to_fileio_pread.  */
+/* Helper for the implementation of to_fileio_pread.  Read the file
+   from the remote side with vFile:pread.  */
 
 static int
-remote_hostio_pread (struct target_ops *self,
-		     int fd, gdb_byte *read_buf, int len,
-		     ULONGEST offset, int *remote_errno)
+remote_hostio_pread_vFile (struct target_ops *self,
+			   int fd, gdb_byte *read_buf, int len,
+			   ULONGEST offset, int *remote_errno)
 {
   struct remote_state *rs = get_remote_state ();
   char *p = rs->buf;
@@ -10461,6 +10520,76 @@ remote_hostio_pread (struct target_ops *self,
   return ret;
 }
 
+/* Serve pread from the readahead cache.  Returns number of bytes
+   read, or 0 if the request can't be served from the cache.  */
+
+static int
+remote_hostio_pread_from_cache (struct remote_state *rs,
+				int fd, gdb_byte *read_buf, size_t len,
+				ULONGEST offset)
+{
+  struct readahead_cache *cache = &rs->readahead_cache;
+
+  if (cache->fd == fd
+      && cache->offset <= offset
+      && offset < cache->offset + cache->bufsize)
+    {
+      ULONGEST max = cache->offset + cache->bufsize;
+
+      if (offset + len > max)
+	len = max - offset;
+
+      memcpy (read_buf, cache->buf + offset - cache->offset, len);
+      return len;
+    }
+
+  return 0;
+}
+
+/* Implementation of to_fileio_pread.  */
+
+static int
+remote_hostio_pread (struct target_ops *self,
+		     int fd, gdb_byte *read_buf, int len,
+		     ULONGEST offset, int *remote_errno)
+{
+  int ret;
+  struct remote_state *rs = get_remote_state ();
+  struct readahead_cache *cache = &rs->readahead_cache;
+
+  ret = remote_hostio_pread_from_cache (rs, fd, read_buf, len, offset);
+  if (ret > 0)
+    {
+      cache->hit_count++;
+
+      if (remote_debug)
+	fprintf_unfiltered (gdb_stdlog, "readahead cache hit %s\n",
+			    pulongest (cache->hit_count));
+      return ret;
+    }
+
+  cache->miss_count++;
+  if (remote_debug)
+    fprintf_unfiltered (gdb_stdlog, "readahead cache miss %s\n",
+			pulongest (cache->miss_count));
+
+  cache->fd = fd;
+  cache->offset = offset;
+  cache->bufsize = get_remote_packet_size ();
+  cache->buf = xrealloc (cache->buf, cache->bufsize);
+
+  ret = remote_hostio_pread_vFile (self, cache->fd, cache->buf, cache->bufsize,
+				   cache->offset, remote_errno);
+  if (ret <= 0)
+    {
+      readahead_cache_invalidate_fd (fd);
+      return ret;
+    }
+
+  cache->bufsize = ret;
+  return remote_hostio_pread_from_cache (rs, fd, read_buf, len, offset);
+}
+
 /* Implementation of to_fileio_close.  */
 
 static int
@@ -10470,6 +10599,8 @@ remote_hostio_close (struct target_ops *self, int fd, int *remote_errno)
   char *p = rs->buf;
   int left = get_remote_packet_size () - 1;
 
+  readahead_cache_invalidate_fd (fd);
+
   remote_buffer_add_string (&p, &left, "vFile:close:");
 
   remote_buffer_add_int (&p, &left, fd);
-- 
1.9.3