From: Stefan Hajnoczi <stefanha@gmail.com>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: vandersonmr <vandersonmr2@gmail.com>,
Riku Voipio <riku.voipio@iki.fi>,
qemu-devel@nongnu.org, Laurent Vivier <laurent@vivier.eu>,
Paolo Bonzini <pbonzini@redhat.com>,
Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH v1 1/2] accel/tcg: adding integration with linux perf
Date: Thu, 22 Aug 2019 15:41:20 +0100 [thread overview]
Message-ID: <20190822144120.GM20491@stefanha-x1.localdomain> (raw)
In-Reply-To: <87lfvummhe.fsf@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 3931 bytes --]
On Thu, Aug 15, 2019 at 05:17:49PM +0100, Alex Bennée wrote:
>
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
> > On Wed, Aug 14, 2019 at 11:37:24PM -0300, vandersonmr wrote:
> >> This commit adds support to Linux Perf in order
> >> to be able to analyze qemu jitted code and
> >> also to able to see the TBs PC in it.
> >
> > Is there any reference to the file format? Please include it in a code
> > comment, if such a thing exists.
> >
> >> diff --git a/accel/tcg/perf/jitdump.c b/accel/tcg/perf/jitdump.c
> >> new file mode 100644
> >> index 0000000000..6f4c0911c2
> >> --- /dev/null
> >> +++ b/accel/tcg/perf/jitdump.c
> >> @@ -0,0 +1,180 @@
> >
> > License header?
> >
> >> +#ifdef __linux__
> >
> > If the entire source file is #ifdef __linux__ then Makefile.objs should
> > probably contain obj-$(CONFIG_LINUX) += jitdump.o instead. Letting the
> > build system decide what to compile is cleaner than ifdeffing large
> > amounts of code.
> >
> >> +
> >> +#include <stdint.h>
> >> +
> >> +#include <stdio.h>
> >> +#include <unistd.h>
> >> +#include <time.h>
> >> +#include <sys/syscall.h>
> >> +#include <elf.h>
> >> +
> >> +#include "jitdump.h"
> >> +#include "qemu-common.h"
> >
> > Please follow QEMU's header ordering conventions. See ./HACKING "1.2.
> > Include directives".
> >
> >> +void start_jitdump_file(void)
> >> +{
> >> + GString *dumpfile_name = g_string_new(NULL);;
> >> + g_string_printf(dumpfile_name, "./jit-%d.dump", getpid());
> >
> > Simpler:
> >
> > gchar *dumpfile_name = g_strdup_printf("./jit-%d.dump", getpid());
> > ...
> > g_free(dumpfile_name);
> >
> >> + dumpfile = fopen(dumpfile_name->str, "w+");
> >
> > getpid() and the global dumpfile variable make me wonder what happens
> > with multi-threaded TCG?
> >
> >> +
> >> + perf_marker = mmap(NULL, sysconf(_SC_PAGESIZE),
> >
> > Please mention the point of this mmap in a comment. My best guess is
> > that perf stores the /proc/$PID/maps and this is how it finds the
> > jitdump file?
> >
> >> + PROT_READ | PROT_EXEC,
> >> + MAP_PRIVATE,
> >> + fileno(dumpfile), 0);
> >> +
> >> + if (perf_marker == MAP_FAILED) {
> >> + printf("Failed to create mmap marker file for perf %d\n", fileno(dumpfile));
> >> + fclose(dumpfile);
> >> + return;
> >> + }
> >> +
> >> + g_string_free(dumpfile_name, TRUE);
> >> +
> >> + struct jitheader *header = g_new0(struct jitheader, 1);
> >
> > Why g_new this struct? It's small and can be declared on the stack.
> >
> > Please use g_free() with g_malloc/new/etc(). It's not safe to mismatch
> > glib and libc memory allocation functions.
> >
> >> + header->magic = 0x4A695444;
> >> + header->version = 1;
> >> + header->elf_mach = get_e_machine();
> >> + header->total_size = sizeof(struct jitheader);
> >> + header->pid = getpid();
> >> + header->timestamp = get_timestamp();
> >> +
> >> + fwrite(header, header->total_size, 1, dumpfile);
> >> +
> >> + free(header);
> >> + fflush(dumpfile);
> >> +}
> >> +
> >> +void append_load_in_jitdump_file(TranslationBlock *tb)
> >> +{
> >> + GString *func_name = g_string_new(NULL);
> >> + g_string_printf(func_name, "TB virt:0x"TARGET_FMT_lx"%c", tb->pc, '\0');
> >
> > The explicit NUL character looks strange to me. I think the idea is to
> > avoid func_name->len + 1? Adding NUL characters to C strings can be a
> > source of bugs, I would stick to convention and do len + 1 instead of
> > putting NUL characters into the GString. This is a question of style
> > though.
>
> The glib functions always add null characters so you shouldn't need to
> manually.
Yep, just remember to do func_name->len + 1 since glib doesn't count the
NUL character that gets added automatically.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2019-08-22 14:45 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-15 2:37 [Qemu-devel] [PATCH v1 0/2] Integrating qemu to Linux Perf vandersonmr
2019-08-15 2:37 ` [Qemu-devel] [PATCH v1 1/2] accel/tcg: adding integration with linux perf vandersonmr
2019-08-15 14:40 ` Stefan Hajnoczi
2019-08-15 16:17 ` Alex Bennée
2019-08-22 14:41 ` Stefan Hajnoczi [this message]
2019-08-21 19:01 ` Vanderson Martins do Rosario
2019-08-22 14:44 ` Stefan Hajnoczi
2019-08-15 2:37 ` [Qemu-devel] [PATCH v1 2/2] tb-stats: adding TBStatistics info into perf dump vandersonmr
2019-08-15 16:19 ` Alex Bennée
2019-08-15 9:14 ` [Qemu-devel] [PATCH v1 0/2] Integrating qemu to Linux Perf no-reply
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=20190822144120.GM20491@stefanha-x1.localdomain \
--to=stefanha@gmail.com \
--cc=alex.bennee@linaro.org \
--cc=laurent@vivier.eu \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=riku.voipio@iki.fi \
--cc=rth@twiddle.net \
--cc=vandersonmr2@gmail.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).