Re: [PATCH 08/10] Don't invalidate dcache when option stack-cache is changed

Doug Evans
2013-11-17T21:44:17Z

On Sat, Nov 2, 2013 at 10:54 PM, Yao Qi <yao@codesourcery.com> wrote:
> Nowadays, option "stack-cache" on->off and off->on transitions trigger
> cache invalidation. It is not very necessary and not friendly to
> using target_dcache for other purpose, such as code caching.
>
> Option "stack-cache" can be regarded as a switch, to decide whether to
> read through cache when reading data from target. When this switch is
> off, read from target. When the switch is on, read from cache. We
> need to keep cache in sync, so when GDB writes data, cache should be
> updated unconditionally.
>
> gdb:
>
> 2013-11-02 Yao Qi <yao@codesourcery.com>
>
> * target-dcache.c (stack_cache_enabled_p_1): Remove.
> (set_stack_cache_enabled_p): Remove.
> (_initialize_target_dcache): Update command register.
> * target.c (memory_xfer_partial_1): Don't check
> stack_cache_enabled.
> ---
> gdb/target-dcache.c | 24 ++++--------------------
> gdb/target.c | 4 ++--
> 2 files changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
> index a840c40..ce36bbb 100644
> --- a/gdb/target-dcache.c
> +++ b/gdb/target-dcache.c
> @@ -100,26 +100,10 @@ target_dcache_get_or_init (void)
> }
>
> /* The option sets this. */
> -static int stack_cache_enabled_p_1 = 1;
> -/* And set_stack_cache_enabled_p updates this.
> - The reason for the separation is so that we don't flush the cache for
> - on->on transitions. */
> -static int stack_cache_enabled_p = 1;
> -
> -/* This is called *after* the stack-cache has been set.
> - Flush the cache for off->on and on->off transitions.
> - There's no real need to flush the cache for on->off transitions,
> - except cleanliness. */
>
> -static void
> -set_stack_cache_enabled_p (char *args, int from_tty,
> - struct cmd_list_element *c)
> -{
> - if (stack_cache_enabled_p != stack_cache_enabled_p_1)
> - target_dcache_invalidate ();
> +static int stack_cache_enabled_p = 1;
>
> - stack_cache_enabled_p = stack_cache_enabled_p_1;
> -}
> +/* Show option "stack-cache". */
>
> static void
> show_stack_cache_enabled_p (struct ui_file *file, int from_tty,
> @@ -143,13 +127,13 @@ void
> _initialize_target_dcache (void)
> {
> add_setshow_boolean_cmd ("stack-cache", class_support,
> - &stack_cache_enabled_p_1, _("\
> + &stack_cache_enabled_p, _("\
> Set cache use for stack access."), _("\
> Show cache use for stack access."), _("\
> When on, use the data cache for all stack access, regardless of any\n\
> configured memory regions. This improves remote performance significantly.\n\
> By default, caching for stack access is on."),
> - set_stack_cache_enabled_p,
> + NULL,
> show_stack_cache_enabled_p,
> &setlist, &showlist);
>

I'm still not comfortable with this.
This is optimizing a rare occurrence. Users aren't expected to be
turning this on and off during a session.

If one wanted to remove the cache invalidation for the off->on
transition that seems reasonable, but if I do a backtrace, turn the
caching optimization off, and then do another backtrace, I'd want the
second one to not use the cache. YMMV.

OTOH, if there was a command I could use to flush the cache after
turning off the stack cache optimization, then I could see leaving the
cache alone after the on->off transition. [I still think it'd be
preferable to invalidate the cache, but I can live with asking users
to live with having to manually flush the cache after turning off the
optimization.]

> diff --git a/gdb/target.c b/gdb/target.c
> index 1212347..55c5d78 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1593,14 +1593,14 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
>
> /* Make sure the cache gets updated no matter what - if we are writing
> to the stack. Even if this write is not tagged as such, we still need
> - to update the cache. */
> + to update the cache. Update the cache to keep it in sync if it
> + has been initialized. */
>
> if (res > 0
> && inf != NULL
> && writebuf != NULL
> && target_dcache_init_p ()
> && !region->attrib.cache
> - && stack_cache_enabled ()
> && object != TARGET_OBJECT_STACK_MEMORY)
> {
> DCACHE *dcache = target_dcache_get ();

This part of the patch seems ok though.