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

[Akihiko Odaki]

On 2024/08/14 4:20, Pierrick Bouvier wrote:
> Hi Akihiko, and thanks for contributing this new plugin.

Hi,

Thanks for reviewing

> Recently, plugins documentation has been modified, and list of plugins 
> and their doc is now in "docs/about/emulation.rst". You may want to 
> rebase on top of master.

I see. I'll rebase and update the documentation with v2.

> Globally, I'm ok with this plugin and the implementation. Just a few 
> fixes are needed for concurrent accesses.
>
> On 8/12/24 23:46, 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.
>
> I think it can be confusing to have two plugins named bb. How about 
> simpoint, or bbv?

How about renaming tests/tcg/plugins/bb.c to simple-bb.c instead and 
keep the name of contrib/plugins/bb.c concise?

tests/tcg/plugins/bb.c is simple and good as a sample, but less useful 
in practice than this plugin due to differences described earlier. On 
the other hand, this plugin is designed to be utilized for practical 
purpose so I want to keep its name short and save typing.

> > [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>
> > ---
> >   docs/devel/tcg-plugins.rst |  20 ++++++
> >   contrib/plugins/bb.c       | 153 
> > +++++++++++++++++++++++++++++++++++++++++++++
> >   contrib/plugins/Makefile   |   1 +
> >   3 files changed, 174 insertions(+)
> >
> > diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
> > index 9cc09d8c3da1..2859eecc13b9 100644
> > --- a/docs/devel/tcg-plugins.rst
> > +++ b/docs/devel/tcg-plugins.rst
> > @@ -332,6 +332,26 @@ run::
> >     160          1      0
> >     135          1      0
> > +- contrib/plugins/bb.c
> > +
> > +The bb plugin allows you to generates basic block vectors for use 
> > with the
> > +`SimPoint <https://cseweb.ucsd.edu/~calder/simpoint/>`__ analysis tool.
> > +
> > +It has two options, ``interval`` and ``outfile``. ``interval`` 
> > specifies the
> > +interval to generate a basic block vector by the number of 
> > instructions. It is
> > +optional, and its default value is 100000000. ``outfile`` is the path to
> > +output files, and it will be suffixed with ``.N.bb`` where ``N`` is a 
> > vCPU
> > +index.
> > +
> > +Example::
> > +
> > +  $ qemu-aarch64 \
> > +    -plugin contrib/plugins/libb.so,interval=100,outfile=sha1 \
> > +    tests/tcg/aarch64-linux-user/sha1
> > +  SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
> > +  $ du sha1.0.bb
> > +  23128   sha1.0.bb
> > +
> >   - contrib/plugins/hotblocks.c
> >   The hotblocks plugin allows you to examine the where hot paths of
> > diff --git a/contrib/plugins/bb.c b/contrib/plugins/bb.c
> > new file mode 100644
> > index 000000000000..4f1266d07ff5
> > --- /dev/null
> > +++ b/contrib/plugins/bb.c
> > @@ -0,0 +1,153 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
>
> A brief summary and the link to simpoint page can be added as a comment 
> here.

I'll add them with v2.

> > +#include <stdio.h>
> > +#include <glib.h>
> > +
> > +#include <qemu-plugin.h>
> > +
> > +typedef struct Bb {
> > +    struct qemu_plugin_scoreboard *count;
> > +    unsigned int index;
> > +} Bb;
> > +
> > +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> > +static GHashTable *bbs;
> > +static GPtrArray *files;
> > +static char *filename;
> > +static struct qemu_plugin_scoreboard *count;
> > +static uint64_t interval = 100000000;
> > +
> > +static void plugin_exit(qemu_plugin_id_t id, void *p)
> > +{
> > +    g_hash_table_unref(bbs);
> > +    g_ptr_array_unref(files);
> > +    g_free(filename);
> > +    qemu_plugin_scoreboard_free(count);
> > +}
> > +
> > +static void free_bb(void *data)
> > +{
> > +    qemu_plugin_scoreboard_free(((Bb *)data)->count);
> > +    g_free(data);
> > +}
> > +
> > +static void free_file(void *data)
> > +{
> > +    fclose(data);
> > +}
> > + > +static directly count_u64(void)
> > +{
> > +    return qemu_plugin_scoreboard_u64(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;
> > +
> > +    if (vcpu_index >= files->len) {
> > +        g_ptr_array_set_size(files, vcpu_index + 1);
> > +    } else if (g_ptr_array_index(files, vcpu_index)) {
> > +        return;
> > +    }
> > +
>
> You need a lock for files array for expansion/access.

I will replace GPtrArray with scoreboard instead.

> > +    vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index);
> > +    g_ptr_array_index(files, vcpu_index) = fopen(vcpu_filename, "w");
> > +}
> > +
> > +static void vcpu_tb_exec(unsigned int vcpu_index, void *udata)
>
> vcpu_tb_exec is a confusing name, as it is called when interval of insn 
> is executed. vcpu_interval_exec would be more clear.

That makes sense. I will do so with the next version.

> > +{
> > +    FILE *file = g_ptr_array_index(files, vcpu_index);
>
> Need lock when accessing this array.
>
> > +    uint64_t count = qemu_plugin_u64_get(count_u64(), vcpu_index) - 
> > interval;
> > +    GHashTableIter iter;
> > +    void *value;
> > +
> > +    if (!file) {
> > +        return;
> > +    }
> > +
> > +    qemu_plugin_u64_set(count_u64(), vcpu_index, count);
> > + > +    fputc('T', file);
>
> Maybe you can add a comment with a link to 
> https://valgrind.org/docs/manual/bbv-manual.html at this point. It's 
> already mentioned in the commit message, but IMHO, it's convenient to 
> have it here, so we know which data is written exactly.

SimPoint should be referred to know the data format. I cited Valgrind 
only to show the popularity of the format and to motivate its 
implementation for QEMU.

> > +
> > +    g_hash_table_iter_init(&iter, bbs);
> > +
> The access to this hashtable should be under a dedicated lock.

I will add a lock with v2.

Regards,
Akihiko Odaki