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

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

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.

>
>> +
>> +    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)?


--
Alex Bennée