| Message ID | 1467058970-62136-1-git-send-email-donb@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 06/27/2016 09:22 PM, Don Breazeal wrote: >>> +/* The default implementation for the to_get_memory_xfer_limit method. >>> + The hard-coded limit here was determined to be a reasonable default >>> + that eliminated exponential slowdown on very large transfers without >>> + unduly compromising performance on smaller transfers. */ >> >> Where's this coming from? Is this new experimentation you did, >> or are you talking about Anton's patch? > > Both. I did some experimentation to verify that things were significantly > slower without any memory transfer limit, which they were, although I never > reproduced the extreme scenario Anton had reported. Presumably the > performance differences were due to hardware and environment differences. > Regarding the comment, I thought some explanation of the hard-coded number > was appropriate. Is there a better or more preferable way to do this, e.g. > refer to the commit hash, or does it just seem superfluous? OK, you didn't mention this experimentation, which left me wondering. Particularly, the mention of "exponential" is what most made me pause, as it's a qualifier not mentioned elsewhere. I guess my main problem with the comment is that by reading it in isolation, one has no clue of how what would cause the slowdown (normally transferring more at a time is faster!), and thus how to reevaluate the default in the future. How about extending to something like: /* The default implementation for the to_get_memory_xfer_limit method. The hard-coded limit here was determined to be a reasonable default that eliminated exponential slowdown on very large transfers without unduly compromising performance on smaller transfers. This slowdown is mostly caused by memory writing routines doing unnecessary work upfront when large requests end up usually only partially satisfied. See memory_xfer_partial's handling of breakpoint shadows. */ Actually, I was going to approve this with that change, but another another thought crossed my mind, sorry... I assume you did this experimentation with remote targets? But this default will never be used with those, so that experimentation is meaningless for native targets? Actually, the whole capping is probably pointless with native targets, since there's really no marshalling and thus no limit. That'd suggest making the target method return "-1" or some such to indicate there's no limit. WDTY? Thanks, Pedro Alves
On 6/30/2016 10:06 AM, Pedro Alves wrote: > On 06/27/2016 09:22 PM, Don Breazeal wrote: > >>>> +/* The default implementation for the to_get_memory_xfer_limit method. >>>> + The hard-coded limit here was determined to be a reasonable default >>>> + that eliminated exponential slowdown on very large transfers without >>>> + unduly compromising performance on smaller transfers. */ >>> >>> Where's this coming from? Is this new experimentation you did, >>> or are you talking about Anton's patch? >> >> Both. I did some experimentation to verify that things were significantly >> slower without any memory transfer limit, which they were, although I never >> reproduced the extreme scenario Anton had reported. Presumably the >> performance differences were due to hardware and environment differences. >> Regarding the comment, I thought some explanation of the hard-coded number >> was appropriate. Is there a better or more preferable way to do this, e.g. >> refer to the commit hash, or does it just seem superfluous? > > OK, you didn't mention this experimentation, which left me wondering. > Particularly, the mention of "exponential" is what most made me pause, > as it's a qualifier not mentioned elsewhere. I should have used something like "significant" or "extreme" instead of exponential. > > I guess my main problem with the comment is that by reading it in > isolation, one has no clue of how what would cause the slowdown (normally > transferring more at a time is faster!), and thus how to reevaluate > the default in the future. How about extending to something like: > > /* The default implementation for the to_get_memory_xfer_limit method. > The hard-coded limit here was determined to be a reasonable default > that eliminated exponential slowdown on very large transfers without > unduly compromising performance on smaller transfers. > This slowdown is mostly caused by memory writing routines doing > unnecessary work upfront when large requests end up usually > only partially satisfied. See memory_xfer_partial's handling of > breakpoint shadows. */ > > Actually, I was going to approve this with that change, but another > another thought crossed my mind, sorry... > > I assume you did this experimentation with remote targets? But this default > will never be used with those, so that experimentation is meaningless for > native targets? Actually, the whole capping is probably pointless with > native targets, since there's really no marshalling and thus no limit. > That'd suggest making the target method return "-1" or some such > to indicate there's no limit. WDTY? That makes sense to me. If it returns ULONGEST_MAX then the rest of the patch can stay as-is. Something like this? +/* The default implementation for the to_get_memory_xfer_limit method. + The default limit is essentially "no limit". */ + +static ULONGEST +default_get_memory_xfer_limit (struct target_ops *self) +{ + return ULONGEST_MAX; +} + Thanks --Don
On 06/30/2016 06:45 PM, Don Breazeal wrote: > That makes sense to me. If it returns ULONGEST_MAX then the rest of the > patch can stay as-is. Something like this? > > +/* The default implementation for the to_get_memory_xfer_limit method. > + The default limit is essentially "no limit". */ > + > +static ULONGEST > +default_get_memory_xfer_limit (struct target_ops *self) > +{ > + return ULONGEST_MAX; > +} Agreed. Though if you use TARGET_DEFAULT_RETURN, then you don't even need that function: /* Return the limit on the size of any single memory transfer for the target. The default limit is essentially "no limit". */ ULONGEST (*to_get_memory_xfer_limit) (struct target_ops *) TARGET_DEFAULT_RETURN (ULONGEST_MAX); Thanks, Pedro Alves
diff --git a/gdb/remote.c b/gdb/remote.c index 501f3c6..dfa41ef 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -10160,6 +10160,14 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object, return TARGET_XFER_OK; } +/* Implementation of to_get_memory_xfer_limit. */ + +static ULONGEST +remote_get_memory_xfer_limit (struct target_ops *ops) +{ + return get_memory_write_packet_size (); +} + static int remote_search_memory (struct target_ops* ops, CORE_ADDR start_addr, ULONGEST search_space_len, @@ -13073,6 +13081,7 @@ Specify the serial device it is connected to\n\ remote_ops.to_interrupt = remote_interrupt; remote_ops.to_pass_ctrlc = remote_pass_ctrlc; remote_ops.to_xfer_partial = remote_xfer_partial; + remote_ops.to_get_memory_xfer_limit = remote_get_memory_xfer_limit; remote_ops.to_rcmd = remote_rcmd; remote_ops.to_pid_to_exec_file = remote_pid_to_exec_file; remote_ops.to_log_command = serial_log_command; diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 03aa2cc..18f22b5 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -2064,6 +2064,27 @@ debug_xfer_partial (struct target_ops *self, enum target_object arg1, const char return result; } +static ULONGEST +delegate_get_memory_xfer_limit (struct target_ops *self) +{ + self = self->beneath; + return self->to_get_memory_xfer_limit (self); +} + +static ULONGEST +debug_get_memory_xfer_limit (struct target_ops *self) +{ + ULONGEST result; + fprintf_unfiltered (gdb_stdlog, "-> %s->to_get_memory_xfer_limit (...)\n", debug_target.to_shortname); + result = debug_target.to_get_memory_xfer_limit (&debug_target); + fprintf_unfiltered (gdb_stdlog, "<- %s->to_get_memory_xfer_limit (", debug_target.to_shortname); + target_debug_print_struct_target_ops_p (&debug_target); + fputs_unfiltered (") = ", gdb_stdlog); + target_debug_print_ULONGEST (result); + fputs_unfiltered ("\n", gdb_stdlog); + return result; +} + static VEC(mem_region_s) * delegate_memory_map (struct target_ops *self) { @@ -4223,6 +4244,8 @@ install_delegators (struct target_ops *ops) ops->to_get_thread_local_address = delegate_get_thread_local_address; if (ops->to_xfer_partial == NULL) ops->to_xfer_partial = delegate_xfer_partial; + if (ops->to_get_memory_xfer_limit == NULL) + ops->to_get_memory_xfer_limit = delegate_get_memory_xfer_limit; if (ops->to_memory_map == NULL) ops->to_memory_map = delegate_memory_map; if (ops->to_flash_erase == NULL) @@ -4454,6 +4477,7 @@ install_dummy_methods (struct target_ops *ops) ops->to_goto_bookmark = tdefault_goto_bookmark; ops->to_get_thread_local_address = tdefault_get_thread_local_address; ops->to_xfer_partial = tdefault_xfer_partial; + ops->to_get_memory_xfer_limit = default_get_memory_xfer_limit; ops->to_memory_map = tdefault_memory_map; ops->to_flash_erase = tdefault_flash_erase; ops->to_flash_done = tdefault_flash_done; @@ -4610,6 +4634,7 @@ init_debug_target (struct target_ops *ops) ops->to_goto_bookmark = debug_goto_bookmark; ops->to_get_thread_local_address = debug_get_thread_local_address; ops->to_xfer_partial = debug_xfer_partial; + ops->to_get_memory_xfer_limit = debug_get_memory_xfer_limit; ops->to_memory_map = debug_memory_map; ops->to_flash_erase = debug_flash_erase; ops->to_flash_done = debug_flash_done; diff --git a/gdb/target.c b/gdb/target.c index 6f69ac3..57202b4 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -60,6 +60,8 @@ static int default_region_ok_for_hw_watchpoint (struct target_ops *, static void default_rcmd (struct target_ops *, const char *, struct ui_file *); +static ULONGEST default_get_memory_xfer_limit (struct target_ops *self); + static ptid_t default_get_ada_task_ptid (struct target_ops *self, long lwp, long tid); @@ -623,6 +625,17 @@ default_terminal_info (struct target_ops *self, const char *args, int from_tty) printf_unfiltered (_("No saved terminal information.\n")); } +/* The default implementation for the to_get_memory_xfer_limit method. + The hard-coded limit here was determined to be a reasonable default + that eliminated exponential slowdown on very large transfers without + unduly compromising performance on smaller transfers. */ + +static ULONGEST +default_get_memory_xfer_limit (struct target_ops *self) +{ + return 4096; +} + /* A default implementation for the to_get_ada_task_ptid target method. This function builds the PTID by using both LWP and TID as part of @@ -1301,8 +1314,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 a limit specified by the target + to mitigate this. */ + len = min (ops->to_get_memory_xfer_limit (ops), len); buf = (gdb_byte *) xmalloc (len); old_chain = make_cleanup (xfree, buf); diff --git a/gdb/target.h b/gdb/target.h index 6b5b6e0..84f12a9 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -745,6 +745,12 @@ struct target_ops ULONGEST *xfered_len) TARGET_DEFAULT_RETURN (TARGET_XFER_E_IO); + /* Return the limit on the size of any single memory transfer + for the target. */ + + ULONGEST (*to_get_memory_xfer_limit) (struct target_ops *) + TARGET_DEFAULT_FUNC (default_get_memory_xfer_limit); + /* Returns the memory map for the target. A return value of NULL means that no memory map is available. If a memory address does not fall within any returned regions, it's assumed to be