Re: [PATCH 05/10] Invalidate or shrink dcache when setting is changed.

Doug Evans
2013-11-17T20:34:44Z

On Sat, Nov 2, 2013 at 10:54 PM, Yao Qi <yao@codesourcery.com> wrote:
> Nowadays, when cache size or line size is changed by command,
> 'target_dcache' is invalidated. It is too conservative. We can
> optimize in the following ways,
>
> - Don't have to invalidate dcache immediately after cache size or
> line size is changed. We can postpone the invalidation to the moment
> using 'target_dcache'.
> - Don't have to invalidate dcache if the cache size is changed. If
> cache size is changed to the value which is still greater than
> dcache's size, nothing should be done. If change to the value
> which is less than dcache's size, just evict cache lines.
>
> This is what this patch does.
>
> gdb:
>
> 2013-11-02 Yao Qi <yao@codesourcery.com>
>
> * dcache.c (dcache_evict): New function.
> (dcache_shrink): New function.
> (dcache_invalidate_p): New function.
> (dcache_alloc): Call dcache_evict.
> (set_dcache_size): Remove call target_dcache_invalidate.
> (set_dcache_line_size): Likewise.
> * dcache.h (dcache_shrink): Declare.
> (dcache_invalidate_p): Declare.
> * target-dcache.c (target_dcache_get): Invalidate and shrink
> cache if necessary.
> (target_dcache_get_or_init): Likewise.
> [...]
> diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
> index bf04679..28f1aa6 100644
> --- a/gdb/target-dcache.c
> +++ b/gdb/target-dcache.c
> @@ -45,6 +45,14 @@ target_dcache_invalidate (void)
> DCACHE *
> target_dcache_get (void)
> {
> + if (target_dcache_init_p ())
> + {
> + if (dcache_invalidate_p (target_dcache))
> + dcache_invalidate (target_dcache);
> +
> + dcache_shrink (target_dcache);
> + }
> +
> return target_dcache;
> }

It feels simpler if this were:

if (target_dcache_init_p ())
dcache_check_resize (target_dcache);
return target_dcache;

There may be a better name than dcache_check_resize, one may want to
call it for situations other than resizings, but here target-dcache.c
is using 3 API routines from dcache.c to achieve what is essentially
one operation: check if the cache needs to be reconfigured and update
if so.

> @@ -54,7 +62,14 @@ target_dcache_get (void)
> DCACHE *
> target_dcache_get_or_init (void)
> {
> - if (!target_dcache_init_p ())
> + if (target_dcache_init_p ())
> + {
> + if (dcache_invalidate_p (target_dcache))
> + dcache_invalidate (target_dcache);
> +
> + dcache_shrink (target_dcache);
> + }
> + else
> target_dcache = dcache_init ();
>
> return target_dcache;

I think it would be better to remove the duplication and have:

if (target_dcache_init_p ())
return target_dcache_get ();
...