From: Pedro Alves <palves@redhat.com>
To: Gary Benson <gbenson@redhat.com>
Cc: "Sandra Loosemore" <sandra@codesourcery.com>,
"Joel Brobecker" <brobecker@adacore.com>,
"Doug Evans" <dje@google.com>,
"Jan Kratochvil" <jan.kratochvil@redhat.com>,
gdb-patches <gdb-patches@sourceware.org>,
"André Pönitz" <apoenitz@t-online.de>,
"Paul Koning" <Paul_Koning@dell.com>
Subject: Re: [PATCH 0/2] Better handling of slow remote transfers
Date: Thu, 20 Aug 2015 14:46:00 -0000 [thread overview]
Message-ID: <55D5E84E.5060303@redhat.com> (raw)
In-Reply-To: <20150819134242.GA18586@blade.nx>
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
next prev parent reply other threads:[~2015-08-20 14:46 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-11 17:22 Doug Evans
2015-08-11 18:55 ` Jan Kratochvil
2015-08-11 19:44 ` Doug Evans
2015-08-11 19:59 ` Joel Brobecker
2015-08-12 9:48 ` Gary Benson
2015-08-12 10:10 ` Pedro Alves
2015-08-12 10:38 ` Gary Benson
2015-08-12 11:29 ` Pedro Alves
2015-08-12 12:32 ` Gary Benson
2015-08-12 12:51 ` Pedro Alves
2015-08-12 13:02 ` Gary Benson
2015-08-12 13:34 ` Pedro Alves
2015-08-12 13:38 ` Gary Benson
2015-08-12 13:44 ` Gary Benson
2015-08-12 13:58 ` Pedro Alves
2015-08-12 14:44 ` Pedro Alves
2015-08-12 15:08 ` Gary Benson
2015-08-12 15:31 ` Pedro Alves
2015-08-12 15:45 ` Gary Benson
2015-08-12 13:29 ` Gary Benson
2015-08-14 18:26 ` Joel Brobecker
2015-08-14 22:26 ` Sandra Loosemore
2015-08-16 18:49 ` Joel Brobecker
2015-08-17 8:53 ` Gary Benson
2015-08-17 14:26 ` Sandra Loosemore
2015-08-18 9:59 ` Gary Benson
2015-08-18 16:52 ` Sandra Loosemore
2015-08-19 1:27 ` Pedro Alves
2015-08-19 10:41 ` [PATCH] Prelimit number of bytes to read in "vFile:pread:" Gary Benson
2015-08-19 10:51 ` Gary Benson
2015-08-19 12:00 ` Pedro Alves
2015-08-19 16:43 ` Sandra Loosemore
2015-08-19 17:21 ` Gary Benson
2015-08-19 21:14 ` Sandra Loosemore
2015-08-20 14:48 ` Pedro Alves
2015-08-20 15:52 ` Pedro Alves
2015-08-20 17:00 ` Pedro Alves
2015-08-20 18:23 ` Sandra Loosemore
2015-08-21 14:52 ` [PATCH] remote: allow aborting long operations (e.g., file transfers) (Re: [PATCH] Prelimit number of bytes to read in "vFile:pread:") Pedro Alves
2015-08-21 17:12 ` Sandra Loosemore
2015-08-21 17:27 ` Pedro Alves
2015-08-25 10:57 ` Pedro Alves
2015-08-25 15:36 ` Pedro Alves
2015-08-25 20:19 ` GDB 7.10 release tentative date: Fri Aug 28 (was: "Re: [PATCH] remote: allow aborting long operations (e.g., file transfers) (Re: [PATCH] Prelimit number of bytes to read in "vFile:pread:")") Joel Brobecker
2015-08-24 8:45 ` [PATCH] remote: allow aborting long operations (e.g., file transfers) (Re: [PATCH] Prelimit number of bytes to read in "vFile:pread:") Gary Benson
2015-08-19 11:44 ` [PATCH] Prelimit number of bytes to read in "vFile:pread:" Pedro Alves
2015-08-19 13:07 ` [pushed] " Gary Benson
2015-08-19 13:42 ` [PATCH 0/2] Better handling of slow remote transfers Gary Benson
2015-08-20 14:46 ` Pedro Alves [this message]
2015-08-20 18:01 ` Gary Benson
2015-08-21 9:34 ` [pushed] Add readahead cache to gdb's vFile:pread (Re: [PATCH 0/2] Better handling of slow remote transfers) Pedro Alves
2015-08-11 20:00 ` [PATCH 0/2] Better handling of slow remote transfers Jan Kratochvil
2015-08-12 10:05 ` Pedro Alves
-- strict thread matches above, loose matches on Subject: below --
2015-08-05 15:28 Gary Benson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55D5E84E.5060303@redhat.com \
--to=palves@redhat.com \
--cc=Paul_Koning@dell.com \
--cc=apoenitz@t-online.de \
--cc=brobecker@adacore.com \
--cc=dje@google.com \
--cc=gbenson@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=jan.kratochvil@redhat.com \
--cc=sandra@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).