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

[Doug Evans <dje@google.com>]

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.