Re: [PATCH v1 8/9] plugins: new hwprofile plugin

["Alex Bennée" <alex.bennee@linaro.org>]

Robert Foley <robert.foley@linaro.org> writes:

> Hi,
>
> On Tue, 2 Jun 2020 at 11:46, Alex Bennée <alex.bennee@linaro.org> wrote:
> <snip>
>> diff --git a/tests/plugin/hwprofile.c b/tests/plugin/hwprofile.c
>> new file mode 100644
>> index 00000000000..f5e0639e762
>> --- /dev/null
>> +++ b/tests/plugin/hwprofile.c
> <snip>
>> +static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>> +                       uint64_t vaddr, void *udata)
>> +{
>> +    struct qemu_plugin_hwaddr *hwaddr = qemu_plugin_get_hwaddr(meminfo, vaddr);
>> +
>> +    if (!hwaddr || !qemu_plugin_hwaddr_is_io(hwaddr)) {
>> +        return;
>> +    } else {
>> +        char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>> +        DeviceCounts *counts;
>> +
>> +        g_mutex_lock(&lock);
>> +        counts = (DeviceCounts *) g_hash_table_lookup(devices, name);
>> +        if (!counts) {
>> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
>> +            uint64_t base = vaddr - off;
>> +            counts = new_count(name, base);
>> +        } else {
>> +            g_free(name);
>> +        }
>> +
>> +        if (detail) {
>> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
>> +            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);
>> +            if (!io_count) {
>> +                io_count = new_location(off);
>> +                g_hash_table_insert(counts->access_pattern, &off, io_count);
>> +            }
>> +            if (qemu_plugin_mem_is_store(meminfo)) {
>> +                io_count->writes++;
>> +                io_count->cpu_write |= (1 << cpu_index);
>> +            } else {
>> +                io_count->reads++;
>> +                io_count->cpu_read |= (1 << cpu_index);
>> +            }
>> +        } else {
>> +            if (qemu_plugin_mem_is_store(meminfo)) {
>> +                counts->total_writes++;
>> +                counts->cpu_write |= (1 << cpu_index);
>> +            } else {
>> +                counts->total_reads++;
>> +                counts->cpu_read |= (1 << cpu_index);
>
> The bitmasks cpu_read and cpu_write are ints.  Maybe to account for
> larger core counts

I could make them uint64_t and then just warn if we exceed that on start-up.

>> 32, we could assert if the cpu_index is >= 32?
>
>> +            }
>> +        }
>> +        g_mutex_unlock(&lock);
>> +    }
>> +}
>> +
>> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>> +{
>> +    size_t n = qemu_plugin_tb_n_insns(tb);
>> +    size_t i;
>> +
>> +    for (i = 0; i < n; i++) {
>> +        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>> +        qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
>> +                                         QEMU_PLUGIN_CB_NO_REGS,
>> +                                         rw, NULL);
>> +    }
>> +}
>> +
>> +QEMU_PLUGIN_EXPORT
>> +int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
>> +                        int argc, char **argv)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < argc; i++) {
>> +        char *opt = argv[i];
>> +        if (g_strcmp0(opt, "read") == 0) {
>> +            rw = QEMU_PLUGIN_MEM_R;
>> +        } else if (g_strcmp0(opt, "write") == 0) {
>> +            rw = QEMU_PLUGIN_MEM_W;
>> +        } else if (g_strcmp0(opt, "detail") == 0) {
>
> When testing out the options, I noticed that
> if we supply arguments of "read", and "write", then we will only get
> the last one set, "write", since rw gets overwritten.
> One option would be to error out if more than one of these read/write
> args is supplied.

Yeah the option parsing is a little clunky although given the way you
pass them from the QEMU command line perhaps not too worth finessing.
The default is rw so you make a conscious decision to only care about one
or the other.

All you can really do is fail to initialise the plugin. Hopefully the
output should be enough clue.

>
> Reviewed-by: Robert Foley <robert.foley@linaro.org>
> Tested-by: Robert Foley <robert.foley@linaro.org>

Thanks.

Out of interest what did you measure? Are there any useful use cases you can
think of?

>
>> +            detail = true;
>> +        } else {
>> +            fprintf(stderr, "option parsing failed: %s\n", opt);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    plugin_init();
>> +
>> +    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
>> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
>> +    return 0;
>> +}
>> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
>> index b3250e2504c..d87b8d40699 100644
>> --- a/tests/plugin/Makefile
>> +++ b/tests/plugin/Makefile
>> @@ -14,6 +14,7 @@ NAMES += hotblocks
>>  NAMES += howvec
>>  NAMES += hotpages
>>  NAMES += lockstep
>> +NAMES += hwprofile
>>
>>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>>
>> --
>> 2.20.1
>>


-- 
Alex Bennée