Re: [Qemu-devel] [PATCH v1 1/2] accel/tcg: adding integration with linux perf

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4143 bytes --]

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.

> +
> +    struct jr_code_load *load_event = g_new0(struct jr_code_load, 1);

No need to allocate load_event on the heap.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9621e934c0..1c26eeeb9c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4147,6 +4147,18 @@ STEXI
>  Enable FIPS 140-2 compliance mode.
>  ETEXI
>  
> +#ifdef __linux__
> +DEF("perf", 0, QEMU_OPTION_perf,
> +    "-perf    dump jitdump files to help linux perf JIT code visualization\n",
> +    QEMU_ARCH_ALL)
> +#endif
> +STEXI
> +@item -perf
> +@findex -perf
> +Dumps jitdump files to help linux perf JIT code visualization

Suggestions on expanding the documentation:

Where are the jitdump files dumped?  The current working directory?

Anything to say about the naming scheme for these files?

Can you include an example of how to load them into perf(1)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]