This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PING] [PATCH] Optimize memory_xfer_partial for remote
- From: Don Breazeal <donb at codesourcery dot com>
- To: <gdb-patches at sourceware dot org>
- Date: Mon, 20 Jun 2016 08:31:09 -0700
- Subject: [PING] [PATCH] Optimize memory_xfer_partial for remote
- Authentication-results: sourceware.org; auth=none
- References: <1464980562-24184-1-git-send-email-donb at codesourcery dot com>
Ping
On 6/3/2016 12:02 PM, Don Breazeal wrote:
> Some analysis we did here showed that increasing the cap on the
> transfer size in target.c:memory_xfer_partial could give 20% or more
> improvement in remote load across JTAG. Transfer sizes are capped
> to 4K bytes because of performance problems encountered with the
> restore command, documented here:
>
> https://sourceware.org/ml/gdb-patches/2013-07/msg00611.html
>
> and with commit hash: 67c059c29e1fb0cdeacdd2005f955514d8d1fb34
>
> The 4K cap was introduced because in a case where the restore command
> requested a 100MB transfer, memory_xfer_partial would allocate and copy
> an entire 100MB buffer in order to properly handle breakpoint shadow
> instructions, even though memory_xfer_partial would actually only
> write a small portion of the buffer contents.
>
> A couple of alternative solutions were suggested:
> * change the algorithm for handling the breakpoint shadow instructions
> * throttle the transfer size up or down based on the previous actual
> transfer size
>
> I tried implementing the throttling approach, and my implementation
> reduced the performance in some cases.
>
> This patch takes a simple approach: instead of hard-coding the cap on
> transfer requests to 4096, use a variable and allow the target to set it.
> This allows the remote target to set the cap to its packetsize.
>
> Here is the performance for a 100MB restore command using an srec file
> (minutes:seconds), where the remote has a packetsize of 16K bytes:
> * existing implementation: 7:50
> * proposed implementation: 5:34
>
> We could make a similar change in target_read_alloc_1 and
> target_fileio_read_alloc_1, but I left that alone for now.
>
> I considered making target_set_memory_xfer_limit a function in the target
> vector, but concluded that was overkill. In this patch it is an external
> function in target.c.
>
> Tested on x86_64 Linux with native and native-gdbserver, and manually
> tested 'restore' on a Windows 7 host with a bare-metal ARM board.
>
> Thanks,
> --Don
>
> gdb/ChangeLog:
> 2016-06-03 Don Breazeal <donb@codesourcery.com>
>
> * remote.c (remote_start_remote): Call
> target_set_memory_xfer_limit.
> * target.c (memory_xfer_limit): New static variable.
> (target_set_memory_xfer_limit): New function.
> (memory_xfer_partial): Use memory_xfer_limit in place of
> constant.
> * target.h (target_set_memory_xfer_limit): Declare new function.
>
> ---
> gdb/remote.c | 3 +++
> gdb/target.c | 19 +++++++++++++++++--
> gdb/target.h | 6 ++++++
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 1f0d67c..028d555 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -4079,6 +4079,9 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
> getpkt (&rs->buf, &rs->buf_size, 0);
> }
>
> + /* Set the cap on memory transfer requests to our packet size. */
> + target_set_memory_xfer_limit (get_remote_packet_size ());
> +
> /* Let the target know which signals it is allowed to pass down to
> the program. */
> update_signals_program_target ();
> diff --git a/gdb/target.c b/gdb/target.c
> index c0ce46d..dde71c2 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -162,6 +162,10 @@ int may_insert_fast_tracepoints = 1;
>
> int may_stop = 1;
>
> +/* Limit on size of memory transfers, see memory_xfer_partial. */
> +
> +static ULONGEST memory_xfer_limit = 4096;
> +
> /* Non-zero if we want to see trace of target level stuff. */
>
> static unsigned int targetdebug = 0;
> @@ -1235,6 +1239,16 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
> return res;
> }
>
> +/* Set the cap on actual memory transfer requests. This prevents
> + repeated requests to transfer much more than the transport
> + mechanism can accommodate. See memory_xfer_partial. */
> +
> +void
> +target_set_memory_xfer_limit (ULONGEST new_limit)
> +{
> + memory_xfer_limit = new_limit;
> +}
> +
> /* Perform a partial memory transfer. For docs see target.h,
> to_xfer_partial. */
>
> @@ -1269,8 +1283,9 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
> by memory_xfer_partial_1. We will continually malloc
> and free a copy of the entire write request for breakpoint
> shadow handling even though we only end up writing a small
> - subset of it. Cap writes to 4KB to mitigate this. */
> - len = min (4096, len);
> + subset of it. Cap writes to the value of memory_xfer_limit
> + to mitigate this. */
> + len = min (memory_xfer_limit, len);
>
> buf = (gdb_byte *) xmalloc (len);
> old_chain = make_cleanup (xfree, buf);
> diff --git a/gdb/target.h b/gdb/target.h
> index 6b5b6e0..b511746 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -266,6 +266,12 @@ enum target_xfer_status
> const gdb_byte *writebuf, ULONGEST memaddr,
> LONGEST len, ULONGEST *xfered_len);
>
> +/* Set the cap on actual memory transfer requests. This prevents
> + repeated requests to transfer much more than the transport
> + mechanism can accommodate. See memory_xfer_partial. */
> +
> +extern void target_set_memory_xfer_limit (ULONGEST new_limit);
> +
> /* Request that OPS transfer up to LEN addressable units of the target's
> OBJECT. When reading from a memory object, the size of an addressable unit
> is architecture dependent and can be found using
>