[0/2] Better handling of slow remote transfers
Commit Message
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(-)
Comments
Pedro Alves wrote:
> Hey, that was a quick and dirty patch. :-)
I know, no worries :)
> Here's a cleaned up version. WDYT?
[snip
> 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:
[snip]
> 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.
Very nice, please commit!
Thanks,
Gary
On 08/20/2015 07:01 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> Hey, that was a quick and dirty patch. :-)
>
> I know, no worries :)
>
>> Here's a cleaned up version. WDYT?
> [snip
>> 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:
> [snip]
>> 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.
>
> Very nice, please commit!
Pushed, master and 7.10.
Thanks,
Pedro Alves
@@ -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);