From: Mahmoud Mandour <ma.mandourr@gmail.com>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: "Emilio G. Cota" <cota@braap.org>,
qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [RFC PATCH v2 2/3] plugins: cache: Enabled parameterization and added trace printing
Date: Wed, 2 Jun 2021 06:29:58 +0200 [thread overview]
Message-ID: <CAD-LL6jeSwMstiErtwiwVD9TC-tOShqfu7yxJvTZVucg4+6Z0w@mail.gmail.com> (raw)
In-Reply-To: <877djdddri.fsf@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 7031 bytes --]
On Tue, Jun 1, 2021 at 1:29 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> (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?
Alright then, I will have it removed for now and maybe add it if there's a
better
solution for it.
>
> 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;
> }
Applied, thanks
>
> > dcache = cache_init(dblksize, dassoc, dcachesize);
> > icache = cache_init(iblksize, iassoc, icachesize);
>
>
> --
> Alex Bennée
>
[-- Attachment #2: Type: text/html, Size: 9395 bytes --]
next prev parent reply other threads:[~2021-06-02 4:31 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-30 6:37 [RFC PATCH v2 0/3] Cache modelling TCG plugin Mahmoud Mandour
2021-05-30 6:37 ` [RFC PATCH v2 1/3] plugins: Added a new cache modelling plugin Mahmoud Mandour
2021-06-02 3:14 ` Mahmoud Mandour
2021-05-30 6:37 ` [RFC PATCH v2 2/3] plugins: cache: Enabled parameterization and added trace printing Mahmoud Mandour
2021-06-01 11:18 ` Alex Bennée
2021-06-02 4:29 ` Mahmoud Mandour [this message]
2021-06-03 9:39 ` Stefan Hajnoczi
2021-06-02 3:15 ` Mahmoud Mandour
2021-05-30 6:37 ` [RFC PATCH v2 3/3] plugins: cache: Added FIFO and LRU eviction policies Mahmoud Mandour
2021-06-01 12:43 ` Alex Bennée
2021-06-02 5:23 ` Mahmoud Mandour
2021-06-02 3:16 ` Mahmoud Mandour
2021-05-30 6:44 ` [RFC PATCH v2 0/3] Cache modelling TCG plugin Mahmoud Mandour
2021-06-01 13:30 ` Alex Bennée
2021-06-02 6:22 ` Mahmoud Mandour
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAD-LL6jeSwMstiErtwiwVD9TC-tOShqfu7yxJvTZVucg4+6Z0w@mail.gmail.com \
--to=ma.mandourr@gmail.com \
--cc=alex.bennee@linaro.org \
--cc=cota@braap.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).