Re: [RFC PATCH v2 2/3] plugins: cache: Enabled parameterization and added trace printing

["Alex Bennée" <alex.bennee@linaro.org>]

(Stefan CC'ed for tracing discussion)

Mahmoud Mandour <ma.mandourr@gmail.com> writes:

> Made both icache and dcache configurable through plugin arguments
> and added memory trace printing in a separate file.

Please keep the commits discreet and single topic. The memory trace is
an extra feature so should be in it's own commit.

>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  contrib/plugins/cache.c | 68 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> index 8c9d1dd538..fa0bf1dd40 100644
> --- a/contrib/plugins/cache.c
> +++ b/contrib/plugins/cache.c
> @@ -22,7 +22,7 @@ static GRand *rng;
>  static GHashTable *dmiss_ht;
>  static GHashTable *imiss_ht;
>  
> -static GMutex dmtx, imtx;
> +static GMutex dmtx, imtx, fmtx;
>  
>  static int limit;
>  static bool sys;
> @@ -33,6 +33,8 @@ static uint64_t dmisses;
>  static uint64_t imem_accesses;
>  static uint64_t imisses;
>  
> +FILE *tracefile;
> +
>  static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
>  
>  enum AccessResult {
> @@ -205,6 +207,16 @@ static void vcpu_mem_access(unsigned int cpu_index, qemu_plugin_meminfo_t info,
>      insn_addr = ((struct InsnData *) userdata)->addr;
>      effective_addr = hwaddr ? qemu_plugin_hwaddr_phys_addr(hwaddr) : vaddr;
>  
> +    if (tracefile) {
> +        g_mutex_lock(&fmtx);
> +        g_autoptr(GString) rep = g_string_new("");
> +        bool is_store = qemu_plugin_mem_is_store(info);
> +        g_string_append_printf(rep, "%c: 0x%" PRIx64,
> +                is_store ? 'S' : 'L', effective_addr);
> +        fprintf(tracefile, "%s\n", rep->str);
> +        g_mutex_unlock(&fmtx);
> +    }

I can see this would be useful for debugging but I'm wary of adding
ad-hoc tracing formats when QEMU already has support for a wide range of
tracing formats. We discussed this a bit in:

  Subject: trace_FOO_tcg bit-rotted?
  Date: Tue, 06 Apr 2021 17:00:20 +0100
  Message-ID: <87eefnwd0l.fsf@linaro.org>

However I don't know how easy it would be to leverage the existing
tracing infrastructure from inside a plugin. As I understand it QEMU
currently builds a static list of trace points during the build so maybe
we would need additional infrastructure for a plugin to register a trace
point and for the final output to be use-able. For example the binary
trace output I think still needs to reference the source trace-events
file?

So that's not a NACK but maybe we could spend a little time working out
if we can come up with a cleaner solution?

Stefan, any thoughts?

>      if (access_cache(dcache, effective_addr) == MISS) {
>          struct InsnData *insn = get_or_create(dmiss_ht, userdata, insn_addr);
>          insn->misses++;
> @@ -221,11 +233,20 @@ static void vcpu_insn_exec(unsigned int vcpu_index, void *userdata)
>      g_mutex_lock(&imtx);
>      addr = ((struct InsnData *) userdata)->addr;
>  
> +    if (tracefile) {
> +        g_mutex_lock(&fmtx);
> +        g_autoptr(GString) rep = g_string_new("");
> +        g_string_append_printf(rep, "I: 0x%" PRIx64, addr);
> +        fprintf(tracefile, "%s\n", rep->str);
> +        g_mutex_unlock(&fmtx);
> +    }
> +
>      if (access_cache(icache, addr) == MISS) {
>          struct InsnData *insn = get_or_create(imiss_ht, userdata, addr);
>          insn->misses++;
>          imisses++;
>      }
> +
>      imem_accesses++;
>      g_mutex_unlock(&imtx);
>  }
> @@ -352,6 +373,15 @@ static void plugin_exit()
>  
>      g_mutex_unlock(&dmtx);
>      g_mutex_unlock(&imtx);
> +
> +    if (tracefile) {
> +        fclose(tracefile);
> +    }
> +}
> +
> +static bool bad_cache_params(int blksize, int assoc, int cachesize)
> +{
> +    return (cachesize % blksize) != 0 || (cachesize % (blksize * assoc) != 0);
>  }
>  
>  QEMU_PLUGIN_EXPORT
> @@ -377,14 +407,48 @@ int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
>  
>      for (i = 0; i < argc; i++) {
>          char *opt = argv[i];
> -        if (g_str_has_prefix(opt, "limit=")) {
> +        if (g_str_has_prefix(opt, "I=")) {
> +            gchar **toks = g_strsplit(opt + 2, " ", -1);
> +            if (g_strv_length(toks) != 3) {
> +                fprintf(stderr, "option parsing failed: %s\n", opt);
> +                return -1;
> +            }
> +            icachesize = g_ascii_strtoull(toks[0], NULL, 10);
> +            iassoc = g_ascii_strtoull(toks[1], NULL, 10);
> +            iblksize = g_ascii_strtoull(toks[2], NULL, 10);
> +        } else if (g_str_has_prefix(opt, "D=")) {
> +            gchar **toks = g_strsplit(opt + 2, " ", -1);
> +            if (g_strv_length(toks) != 3) {
> +                fprintf(stderr, "option parsing failed: %s\n", opt);
> +                return -1;
> +            }
> +            dcachesize = g_ascii_strtoull(toks[0], NULL, 10);
> +            dassoc = g_ascii_strtoull(toks[1], NULL, 10);
> +            dblksize = g_ascii_strtoull(toks[2], NULL, 10);
> +        } else if (g_str_has_prefix(opt, "limit=")) {
>              limit = g_ascii_strtoull(opt + 6, NULL, 10);
> +        } else if (g_str_has_prefix(opt, "tracefile=")) {
> +            char *file_name = opt + 10;
> +            tracefile = fopen(file_name, "w");
> +            if (!tracefile) {
> +                fprintf(stderr, "could not open: %s for writing\n", file_name);
> +            }
>          } else {
>              fprintf(stderr, "option parsing failed: %s\n", opt);
>              return -1;
>          }
>      }
>  
> +    if (bad_cache_params(iblksize, iassoc, icachesize)) {
> +        fprintf(stderr, "icache cannot be constructed from given parameters\n");
> +        return -1;
> +    }
> +
> +    if (bad_cache_params(dblksize, dassoc, dcachesize)) {
> +        fprintf(stderr, "dcache cannot be constructed from given parameters\n");
> +        return -1;
> +    }
> +

Perhaps roll bad_cache_params into cache_init and return NULL if it
fails, so:

  dcache = cache_init(dblksize, dassoc, dcachesize);
  if (!dcache) {
    fprintf(stderr, "dcache cannot be constructed from given parameters\n");
    return -1;
  }

>      dcache = cache_init(dblksize, dassoc, dcachesize);
>      icache = cache_init(iblksize, iassoc, icachesize);


-- 
Alex Bennée