[PING] [PATCH] Optimize memory_xfer_partial for remote

[Don Breazeal <donb@codesourcery.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
>