Re: [PATCH v9 11/13] accel/tcg: adding integration with linux perf

[Richard Henderson <richard.henderson@linaro.org>]

On 10/7/19 11:28 AM, Alex Bennée wrote:
> +static uint32_t get_e_machine(void)
> +{
> +    uint32_t e_machine = EM_NONE;
> +    Elf64_Ehdr elf_header;

Not ideal, as this appears to not work on 32-bit hosts, but the two structures
do match up within the first 24 bytes, in which this is located.

That said, this value is present within tcg/host/tcg-target.inc.c as
ELF_HOST_MACHINE.  So we really don't have to play /proc/self/exec games.

> +void start_jitdump_file(void)
> +{
> +    g_autofree gchar *dumpfile_name = g_strdup_printf("./jit-%d.dump", getpid());
> +    dumpfile = fopen(dumpfile_name, "w+");
> +
> +    /* 'Perf record' saves mmaped files during the execution of a program and
> +     * 'perf inject' iterate over them to reconstruct all used/executed binary.
> +     * So, we create a mmap with the path of our jitdump that is processed
> +     * and used by 'perf inject' to reconstruct jitted binaries.
> +     */
> +    perf_marker = mmap(NULL, sysconf(_SC_PAGESIZE),
> +                          PROT_READ | PROT_EXEC,
> +                          MAP_PRIVATE,
> +                          fileno(dumpfile), 0);

(1) sysconf(_SC_PAGESIZE) is qemu_real_host_page_size.
(2) This is a page-sized mapping of a new, zero-sized file?
    I assume this mapping event gets logged, and that it its
    only purpose?
(3) I really need to read the kernel docs...

> +void append_load_in_jitdump_file(TranslationBlock *tb)
> +{
> +    gchar *func_name = g_strdup_printf("TB virt:0x"TARGET_FMT_lx, tb->pc);
> +
> +    /* Serialise the writing of the dump file */
> +    qemu_mutex_lock(&dumpfile_lock);
> +
> +    struct jr_code_load load_event;
> +    load_event.p.id = JIT_CODE_LOAD;
> +    load_event.p.total_size =
> +        sizeof(struct jr_code_load) + func_name->len + 1 + tb->tc.size;

How does a "gchar *func_name" have ->len?  Did this used to be GString, but a
last-minute change means it no longer compiles?

> +    fflush(dumpfile);

Why fflushing all of the time?  Surely the file contents doesn't matter until
after the final close.

> +    qemu_mutex_unlock(&dumpfile_lock);

Why a separate qemu locking instead of using stdio's own locking (flockfile).

> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 871d91d559..3fafb656e7 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -58,6 +58,10 @@
>  #include "sysemu/cpus.h"
>  #include "sysemu/tcg.h"
>  
> +#ifdef __linux__
> +#include "perf/jitdump.h"
> +#endif

Why the ifdefs?  We're not dependent on other headers are we?
Not that there's a "perf" on other hosts, but AFACT it should
at least compile...


r~