Re: [PATCH v5 06/12] tests/plugin/mem: migrate to new per_vcpu API

[Pierrick Bouvier]

Hi Luc,

On 2/27/24 1:35 PM, Luc Michel wrote:
> Hi Pierrick,
>
> On 13:14 Mon 26 Feb     , Pierrick Bouvier wrote:
> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > ---
> >   tests/plugin/mem.c | 40 +++++++++++++++++++++++++---------------
> >   1 file changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
> > index 44e91065ba7..d4729f5e015 100644
> > --- a/tests/plugin/mem.c
> > +++ b/tests/plugin/mem.c
> > @@ -16,9 +16,14 @@
> >
> >   QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> >
> > -static uint64_t inline_mem_count;
> > -static uint64_t cb_mem_count;
> > -static uint64_t io_count;
> > +typedef struct {
> > +    uint64_t mem_count;
> > +    uint64_t io_count;
> > +} CPUCount;
> > +
> > +static struct qemu_plugin_scoreboard *counts;
> > +static qemu_plugin_u64 mem_count;
> > +static qemu_plugin_u64 io_count;
>
> I see that you merged inline and callback counts into the same variable.
>
> I wonder... For this test don't you want to keep a plain uint64_t for
> callback counts? I have the feeling that this test was made so one can
> make sure inline and callback counts match.

Indeed, the new API guarantees thread safety (current inline 
implementation was racy), so this plugin now gives the exact same result 
whether you use inline ops or callbacks. In more, callback based 
approach can be implemented without any lock, as we are counting per 
vcpu. Thus, it's faster and safer.

Regarding the "testing" side, this series introduce a new plugin 
tests/plugin/inline.c that allows to test all thoses cases in a single 
plugin. Thus, it's not needed that other plugins offer a way to test this.

Thanks for your review.

> Luc
>
> >   static bool do_inline, do_callback;
> >   static bool do_haddr;
> >   static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
> > @@ -27,16 +32,16 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
> >   {
> >       g_autoptr(GString) out = g_string_new("");
> >
> > -    if (do_inline) {
> > -        g_string_printf(out, "inline mem accesses: %" PRIu64 "\n", 
> > inline_mem_count);
> > -    }
> > -    if (do_callback) {
> > -        g_string_append_printf(out, "callback mem accesses: %" PRIu64 "\n", 
> > cb_mem_count);
> > +    if (do_inline || do_callback) {
> > +        g_string_printf(out, "mem accesses: %" PRIu64 "\n",
> > +                        qemu_plugin_u64_sum(mem_count));
> >       }
> >       if (do_haddr) {
> > -        g_string_append_printf(out, "io accesses: %" PRIu64 "\n", io_count);
> > +        g_string_append_printf(out, "io accesses: %" PRIu64 "\n",
> > +                               qemu_plugin_u64_sum(io_count));
> >       }
> >       qemu_plugin_outs(out->str);
> > +    qemu_plugin_scoreboard_free(counts);
> >   }
> >
> >   static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
> > @@ -46,12 +51,12 @@ static void vcpu_mem(unsigned int cpu_index, 
> > qemu_plugin_meminfo_t meminfo,
> >           struct qemu_plugin_hwaddr *hwaddr;
> >           hwaddr = qemu_plugin_get_hwaddr(meminfo, vaddr);
> >           if (qemu_plugin_hwaddr_is_io(hwaddr)) {
> > -            io_count++;
> > +            qemu_plugin_u64_add(io_count, cpu_index, 1);
> >           } else {
> > -            cb_mem_count++;
> > +            qemu_plugin_u64_add(mem_count, cpu_index, 1);
> >           }
> >       } else {
> > -        cb_mem_count++;
> > +        qemu_plugin_u64_add(mem_count, cpu_index, 1);
> >       }
> >   }
> >
> > @@ -64,9 +69,10 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct 
> > qemu_plugin_tb *tb)
> >           struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> >
> >           if (do_inline) {
> > -            qemu_plugin_register_vcpu_mem_inline(insn, rw,
> > -                                                 QEMU_PLUGIN_INLINE_ADD_U64,
> > -                                                 &inline_mem_count, 1);
> > +            qemu_plugin_register_vcpu_mem_inline_per_vcpu(
> > +                insn, rw,
> > +                QEMU_PLUGIN_INLINE_ADD_U64,
> > +                mem_count, 1);
> >           }
> >           if (do_callback) {
> >               qemu_plugin_register_vcpu_mem_cb(insn, vcpu_mem,
> > @@ -117,6 +123,10 @@ QEMU_PLUGIN_EXPORT int 
> > qemu_plugin_install(qemu_plugin_id_t id,
> >           }
> >       }
> >
> > +    counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
> > +    mem_count = qemu_plugin_scoreboard_u64_in_struct(
> > +        counts, CPUCount, mem_count);
> > +    io_count = qemu_plugin_scoreboard_u64_in_struct(counts, CPUCount, 
> > io_count);
> >       qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> >       qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> >       return 0;
> > --
> > 2.43.0