Optimize memory_xfer_partial for remote

  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(-)
  

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
>
  

On 06/03/2016 08:02 PM, Don Breazeal wrote:

> 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.

Sorry, but that doesn't make sense.  If in the same session you
switch to another target (e.g., core or native debugging), you'll 
continue using the limit set up by the previous remote connection.

There's also an effort to teach gdb about connecting to 
multiple remote targets at the same time.  A global like this
would need to be adjusted to per-connection anyway.

So seems to be we should have a real target_get_memory_xfer_limit()
(or some such) target method.

>  
> +  /* Set the cap on memory transfer requests to our packet size.  */
> +  target_set_memory_xfer_limit (get_remote_packet_size ());

Shouldn't this be based on get_memory_write_packet_size() instead?

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> So seems to be we should have a real target_get_memory_xfer_limit()
> (or some such) target method.

Alternatively, we can move breakpoint_xfer_memory down to each target
(remote, native, etc), on which we know what limit it is.