| Message ID | 20240813-bb-v1-1-effbb77daebf@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | contrib/plugins: Add a plugin to generate basic block vectors | expand |
Hi Akihiko, and thanks for contributing this new plugin. 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. 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? > [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. > +#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. > + 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. > +{ > + 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. > + > + g_hash_table_iter_init(&iter, bbs); > + The access to this hashtable should be under a dedicated lock. > + 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(file, ":%u:%" PRIu64 " ", bb->index, bb_count); > + qemu_plugin_u64_set(bb_count_u64(bb), vcpu_index, 0); > + } > + > + fputc('\n', 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); Need lock for hashtable access. > + Bb *bb = g_hash_table_lookup(bbs, &vaddr); > + > + if (!bb) { > + uint64_t *key = g_new(uint64_t, 1); > + > + *key = vaddr; > + bb = g_new(Bb, 1); > + g_hash_table_insert(bbs, key, bb); > + bb->count = qemu_plugin_scoreboard_new(sizeof(uint64_t)); > + bb->index = g_hash_table_size(bbs); > + } > + > + 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_tb_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); > + files = g_ptr_array_new_with_free_func(free_file); > + count = qemu_plugin_scoreboard_new(sizeof(uint64_t)); > + 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 0b64d2c1e3a9..14bc88bb4f86 100644 > --- a/contrib/plugins/Makefile > +++ b/contrib/plugins/Makefile > @@ -13,6 +13,7 @@ TOP_SRC_PATH = $(SRC_PATH)/../.. > VPATH += $(SRC_PATH) > > NAMES := > +NAMES += bb > NAMES += execlog > NAMES += hotblocks > NAMES += hotpages > > --- > base-commit: 74abb45dac6979e7ff76172b7f0a24e869405184 > change-id: 20240618-bb-93387ddf765b > > Best regards, Regards, Pierrick
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
On 8/13/24 21:56, Akihiko Odaki wrote: > 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. > I would recommend using the <tab> key to save typing. I'm kidding :) More seriously, I get your point, but I don't think this new plugin is generic enough to be qualified as "the" bb plugin (nor the tests/tcg/plugins/bb neither, but it predates this new plugin). It outputs format for SimPoint, so how about simpoint (accessible after typing si<tab>)? >> >>> [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. > Good idea. You'll have to cleanup files at the end manually, as there is no support for custom destructor. >> >>> + 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. > I understood that, but it happens that I didn't find a simple description of the format on SimPoint website (maybe I missed it?), while valgrind doc page is clear and concise. Feel free to point the right page if it exists. >> >>> + >>> + 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
On 2024/08/14 14:41, Pierrick Bouvier wrote: > On 8/13/24 21:56, Akihiko Odaki wrote: >> 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. >> > > I would recommend using the <tab> key to save typing. I'm kidding :) > > More seriously, I get your point, but I don't think this new plugin is > generic enough to be qualified as "the" bb plugin (nor the > tests/tcg/plugins/bb neither, but it predates this new plugin). It > outputs format for SimPoint, so how about simpoint (accessible after > typing si<tab>)? Perhaps bbv is a better naming. I expect there are more use cases of basic block vectors than feeding it for SimPoint. Searching for "basic block vectors" with Google Scholar show this terminology has a consistent meaning and used in several researches. > >>> >>>> [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. >> > > Good idea. You'll have to cleanup files at the end manually, as there is > no support for custom destructor. > >>> >>>> + 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. >> > > I understood that, but it happens that I didn't find a simple > description of the format on SimPoint website (maybe I missed it?), > while valgrind doc page is clear and concise. Feel free to point the > right page if it exists. The authoritative reference would be README.txt included in SimPoint 3.2. The Valgrind documentation is more accessible, but anyone who tries to generate or parse it should refer to SimPoint. Regards, Akihiko Odaki
On 8/13/24 23:32, Akihiko Odaki wrote: > On 2024/08/14 14:41, Pierrick Bouvier wrote: >> On 8/13/24 21:56, Akihiko Odaki wrote: >>> 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. >>> >> >> I would recommend using the <tab> key to save typing. I'm kidding :) >> >> More seriously, I get your point, but I don't think this new plugin is >> generic enough to be qualified as "the" bb plugin (nor the >> tests/tcg/plugins/bb neither, but it predates this new plugin). It >> outputs format for SimPoint, so how about simpoint (accessible after >> typing si<tab>)? > > Perhaps bbv is a better naming. I expect there are more use cases of > basic block vectors than feeding it for SimPoint. Searching for "basic > block vectors" with Google Scholar show this terminology has a > consistent meaning and used in several researches. > bbv works for me
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 */ + +#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 qemu_plugin_u64 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; + } + + 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) +{ + FILE *file = g_ptr_array_index(files, vcpu_index); + 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); + + 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(file, ":%u:%" PRIu64 " ", bb->index, bb_count); + qemu_plugin_u64_set(bb_count_u64(bb), vcpu_index, 0); + } + + fputc('\n', 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); + + if (!bb) { + uint64_t *key = g_new(uint64_t, 1); + + *key = vaddr; + bb = g_new(Bb, 1); + g_hash_table_insert(bbs, key, bb); + bb->count = qemu_plugin_scoreboard_new(sizeof(uint64_t)); + bb->index = g_hash_table_size(bbs); + } + + 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_tb_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); + files = g_ptr_array_new_with_free_func(free_file); + count = qemu_plugin_scoreboard_new(sizeof(uint64_t)); + 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 0b64d2c1e3a9..14bc88bb4f86 100644 --- a/contrib/plugins/Makefile +++ b/contrib/plugins/Makefile @@ -13,6 +13,7 @@ TOP_SRC_PATH = $(SRC_PATH)/../.. VPATH += $(SRC_PATH) NAMES := +NAMES += bb NAMES += execlog NAMES += hotblocks NAMES += hotpages