Re: [PATCH v2] contrib/plugins: Add a plugin to generate basic block vectors

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 8/14/24 22:48, Pierrick Bouvier wrote:
> On 8/14/24 20:04, Akihiko Odaki wrote:
>> SimPoint is a widely used tool to find the ideal microarchitecture
>> simulation points so Valgrind[2] and Pin[3] support generating basic
>> block vectors for use with them. Let's add a corresponding plugin to
>> QEMU too.
>>
>> Note that this plugin has a different goal with tests/plugin/bb.c.
>>
>> This plugin creates a vector for each constant interval instead of
>> counting the execution of basic blocks for the entire run and able to
>> describe the change of execution behavior. Its output is also
>> syntactically simple and better suited for parsing, while the output of
>> tests/plugin/bb.c is more human-readable.
>>
>> [1] https://cseweb.ucsd.edu/~calder/simpoint/
>> [2] https://valgrind.org/docs/manual/bbv-manual.html
>> [3] https://www.intel.com/content/www/us/en/developer/articles/tool/pin-a-dynamic-binary-instrumentation-tool.html
>>
>> Signed-off-by: Yotaro Nada <yotaro.nada@gmail.com>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> Changes in v2:
>> - Merged files variable into the global scoreboard.
>> - Added a lock for bbs.
>> - Added a summary to contrib/plugins/bbv.c.
>> - Rebased.
>> - Link to v1: https://lore.kernel.org/r/20240813-bb-v1-1-effbb77daebf@daynix.com
>> ---
>>    docs/about/emulation.rst |  30 +++++++++
>>    contrib/plugins/bbv.c    | 158 +++++++++++++++++++++++++++++++++++++++++++++++
>>    contrib/plugins/Makefile |   1 +
>>    3 files changed, 189 insertions(+)
>>
>> diff --git a/docs/about/emulation.rst b/docs/about/emulation.rst
>> index c03033e4e956..72d7846ab6f8 100644
>> --- a/docs/about/emulation.rst
>> +++ b/docs/about/emulation.rst
>> @@ -381,6 +381,36 @@ run::
>>      160          1      0
>>      135          1      0
>>    
>> +Basic Block Vectors
>> +...................
>> +
>> +``contrib/plugins/bbv.c``
>> +
>> +The bbv plugin allows you to generate basic block vectors for use with the
>> +`SimPoint <https://cseweb.ucsd.edu/~calder/simpoint/>`__ analysis tool.
>> +
>> +.. list-table:: Basic block vectors arguments
>> +  :widths: 20 80
>> +  :header-rows: 1
>> +
>> +  * - Option
>> +    - Description
>> +  * - interval=N
>> +    - The interval to generate a basic block vector specified by the number of
>> +      instructions (Default: N = 100000000)
>> +  * - outfile=PATH
>> +    - The path to output files.
>> +      It will be suffixed with ``.N.bb`` where ``N`` is a vCPU index.
>> +
>> +Example::
>> +
>> +  $ qemu-aarch64 \
>> +    -plugin contrib/plugins/libbbv.so,interval=100,outfile=sha1 \
>> +    tests/tcg/aarch64-linux-user/sha1
>> +  SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
>> +  $ du sha1.0.bb
>> +  23128   sha1.0.bb
>> +
>>    Hot Blocks
>>    ..........
>>    
>> diff --git a/contrib/plugins/bbv.c b/contrib/plugins/bbv.c
>> new file mode 100644
>> index 000000000000..41139f423fe2
>> --- /dev/null
>> +++ b/contrib/plugins/bbv.c
>> @@ -0,0 +1,158 @@
>> +/*
>> + * Generate basic block vectors for use with the SimPoint analysis tool.
>> + * SimPoint: https://cseweb.ucsd.edu/~calder/simpoint/
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include <stdio.h>
>> +#include <glib.h>
>> +
>> +#include <qemu-plugin.h>
>> +
>> +typedef struct Bb {
>> +    struct qemu_plugin_scoreboard *count;
>> +    unsigned int index;
>> +} Bb;
>> +
>> +typedef struct Vcpu {
>> +    uint64_t count;
>> +    FILE *file;
>> +} Vcpu;
>> +
>> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>> +static GHashTable *bbs;
>> +static GRWLock bbs_lock;
>> +static char *filename;
>> +static struct qemu_plugin_scoreboard *vcpus;
>> +static uint64_t interval = 100000000;
>> +
>> +static void plugin_exit(qemu_plugin_id_t id, void *p)
>> +{
>> +    for (int i = 0; i < qemu_plugin_num_vcpus(); i++) {
>> +        fclose(((Vcpu *)qemu_plugin_scoreboard_find(vcpus, i))->file);
>> +    }
>> +
>> +    g_hash_table_unref(bbs);
>> +    g_free(filename);
>> +    qemu_plugin_scoreboard_free(vcpus);
>> +}
>> +
>> +static void free_bb(void *data)
>> +{
>> +    qemu_plugin_scoreboard_free(((Bb *)data)->count);
>> +    g_free(data);
>> +}
>> +
>> +static qemu_plugin_u64 count_u64(void)
>> +{
>> +    return qemu_plugin_scoreboard_u64_in_struct(vcpus, Vcpu, count);
>> +}
>> +
>> +static qemu_plugin_u64 bb_count_u64(Bb *bb)
>> +{
>> +    return qemu_plugin_scoreboard_u64(bb->count);
>> +}
>> +
>> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
>> +{
>> +    g_autofree gchar *vcpu_filename = NULL;
>> +    Vcpu *vcpu = qemu_plugin_scoreboard_find(vcpus, vcpu_index);
>> +
>> +    vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index);
>> +    vcpu->file = fopen(vcpu_filename, "w");
>> +}
>> +
>> +static void vcpu_interval_exec(unsigned int vcpu_index, void *udata)
>> +{
>> +    Vcpu *vcpu = qemu_plugin_scoreboard_find(vcpus, vcpu_index);
>> +    GHashTableIter iter;
>> +    void *value;
>> +
>> +    if (!vcpu->file) {
>> +        return;
>> +    }
>> +
>> +    vcpu->count -= interval;
>> +
>> +    fputc('T', vcpu->file);
>> +
>> +    g_rw_lock_reader_lock(&bbs_lock);
>> +    g_hash_table_iter_init(&iter, bbs);
>> +
>> +    while (g_hash_table_iter_next(&iter, NULL, &value)) {
>> +        Bb *bb = value;
>> +        uint64_t bb_count = qemu_plugin_u64_get(bb_count_u64(bb), vcpu_index);
>> +
>> +        if (!bb_count) {
>> +            continue;
>> +        }
>> +
>> +        fprintf(vcpu->file, ":%u:%" PRIu64 " ", bb->index, bb_count);
>> +        qemu_plugin_u64_set(bb_count_u64(bb), vcpu_index, 0);
>> +    }
>> +
>> +    g_rw_lock_reader_unlock(&bbs_lock);
>> +    fputc('\n', vcpu->file);
>> +}
>> +
>> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>> +{
>> +    uint64_t n_insns = qemu_plugin_tb_n_insns(tb);
>> +    uint64_t vaddr = qemu_plugin_tb_vaddr(tb);
>> +    Bb *bb = g_hash_table_lookup(bbs, &vaddr);
> 
> Missing a read_lock for this access.
> 
>> +
>> +    if (!bb) {
>> +        uint64_t *key = g_new(uint64_t, 1);
>> +
>> +        *key = vaddr;
>> +        bb = g_new(Bb, 1);
>> +        bb->count = qemu_plugin_scoreboard_new(sizeof(uint64_t));
>> +        bb->index = g_hash_table_size(bbs);

And querying the size should be protected under the writer_lock too. (or 
another read_lock, but that would be less efficient).

>> +        g_rw_lock_writer_lock(&bbs_lock);
>> +        g_hash_table_insert(bbs, key, bb);
>> +        g_rw_lock_writer_unlock(&bbs_lock);
>> +    }
>> +
>> +    qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
>> +        tb, QEMU_PLUGIN_INLINE_ADD_U64, count_u64(), n_insns);
>> +
>> +    qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
>> +        tb, QEMU_PLUGIN_INLINE_ADD_U64, bb_count_u64(bb), n_insns);
>> +
>> +    qemu_plugin_register_vcpu_tb_exec_cond_cb(
>> +        tb, vcpu_interval_exec, QEMU_PLUGIN_CB_NO_REGS,
>> +        QEMU_PLUGIN_COND_GE, count_u64(), interval, NULL);
>> +}
>> +
>> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>> +                                           const qemu_info_t *info,
>> +                                           int argc, char **argv)
>> +{
>> +    for (int i = 0; i < argc; i++) {
>> +        char *opt = argv[i];
>> +        g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
>> +        if (g_strcmp0(tokens[0], "interval") == 0) {
>> +            interval = g_ascii_strtoull(tokens[1], NULL, 10);
>> +        } else if (g_strcmp0(tokens[0], "outfile") == 0) {
>> +            filename = tokens[1];
>> +            tokens[1] = NULL;
>> +        } else {
>> +            fprintf(stderr, "option parsing failed: %s\n", opt);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    if (!filename) {
>> +        fputs("outfile unspecified\n", stderr);
>> +        return -1;
>> +    }
>> +
>> +    bbs = g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, free_bb);
>> +    vcpus = qemu_plugin_scoreboard_new(sizeof(Vcpu));
>> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
>> +    qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
>> +    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
>> +
>> +    return 0;
>> +}
>> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
>> index edf256cd9d11..6936591b1022 100644
>> --- a/contrib/plugins/Makefile
>> +++ b/contrib/plugins/Makefile
>> @@ -13,6 +13,7 @@ TOP_SRC_PATH = $(SRC_PATH)/../..
>>    VPATH += $(SRC_PATH)
>>    
>>    NAMES :=
>> +NAMES += bbv
>>    NAMES += execlog
>>    NAMES += hotblocks
>>    NAMES += hotpages
>>
>> ---
>> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
>> change-id: 20240618-bb-93387ddf765b
>>
>> Best regards,
> 
> Otherwise,
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>