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

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

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 👍, thanks.

>>
>>>>
>>>>> [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.
> 

Thanks. I found this link that works, but it's not an official mirror:
https://github.com/hanhwi/SimPoint/blob/9c42d14fbfb90207dde915d705e11101b0e12e9f/README.txt#L153

> Regards,
> Akihiko Odaki