While there isn't any easy way to make the inline counts thread safe
we can ensure the callback based ones are. While we are at it we can
reduce introduce a new option ("idle") to dump a report of the current
bb and insn count each time a vCPU enters the idle state.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Dave Bort <dbort@dbort.com>
---
v2
- fixup for non-inline linux-user case
- minor cleanup and re-factor
---
tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 83 insertions(+), 13 deletions(-)
diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index df19fd359df3..89c373e19cd8 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -16,24 +16,67 @@
QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
-static uint64_t bb_count;
-static uint64_t insn_count;
+typedef struct {
+ GMutex lock;
+ int index;
+ uint64_t bb_count;
+ uint64_t insn_count;
+} CPUCount;
+
+/* Used by the inline & linux-user counts */
static bool do_inline;
+static CPUCount inline_count;
+
+/* Dump running CPU total on idle? */
+static bool idle_report;
+static GPtrArray *counts;
+static int max_cpus;
+
+static void gen_one_cpu_report(CPUCount *count, GString *report)
+{
+ if (count->bb_count) {
+ g_string_append_printf(report, "CPU%d: "
+ "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+ count->index,
+ count->bb_count, count->insn_count);
+ }
+}
static void plugin_exit(qemu_plugin_id_t id, void *p)
{
- g_autofree gchar *out = g_strdup_printf(
- "bb's: %" PRIu64", insns: %" PRIu64 "\n",
- bb_count, insn_count);
- qemu_plugin_outs(out);
+ g_autoptr(GString) report = g_string_new("");
+
+ if (do_inline || !max_cpus) {
+ g_string_printf(report, "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+ inline_count.bb_count, inline_count.insn_count);
+ } else {
+ g_ptr_array_foreach(counts, (GFunc) gen_one_cpu_report, report);
+ }
+ qemu_plugin_outs(report->str);
+}
+
+static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+ CPUCount *count = g_ptr_array_index(counts, cpu_index);
+ g_autoptr(GString) report = g_string_new("");
+ gen_one_cpu_report(count, report);
+
+ if (report->len > 0) {
+ g_string_prepend(report, "Idling ");
+ qemu_plugin_outs(report->str);
+ }
}
static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
{
- unsigned long n_insns = (unsigned long)udata;
+ CPUCount *count = max_cpus ?
+ g_ptr_array_index(counts, cpu_index) : &inline_count;
- insn_count += n_insns;
- bb_count++;
+ unsigned long n_insns = (unsigned long)udata;
+ g_mutex_lock(&count->lock);
+ count->insn_count += n_insns;
+ count->bb_count++;
+ g_mutex_unlock(&count->lock);
}
static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
@@ -42,9 +85,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
if (do_inline) {
qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
- &bb_count, 1);
+ &inline_count.bb_count, 1);
qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
- &insn_count, n_insns);
+ &inline_count.insn_count, n_insns);
} else {
qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
QEMU_PLUGIN_CB_NO_REGS,
@@ -56,8 +99,35 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
const qemu_info_t *info,
int argc, char **argv)
{
- if (argc && strcmp(argv[0], "inline") == 0) {
- do_inline = true;
+ int i;
+
+ for (i = 0; i < argc; i++) {
+ char *opt = argv[i];
+ if (g_strcmp0(opt, "inline") == 0) {
+ do_inline = true;
+ } else if (g_strcmp0(opt, "idle") == 0) {
+ idle_report = true;
+ } else {
+ fprintf(stderr, "option parsing failed: %s\n", opt);
+ return -1;
+ }
+ }
+
+ if (info->system_emulation && !do_inline) {
+ max_cpus = info->system.max_vcpus;
+ counts = g_ptr_array_new();
+ for (i = 0; i < max_cpus; i++) {
+ CPUCount *count = g_new0(CPUCount, 1);
+ g_mutex_init(&count->lock);
+ count->index = i;
+ g_ptr_array_add(counts, count);
+ }
+ } else if (!do_inline) {
+ g_mutex_init(&inline_count.lock);
+ }
+
+ if (idle_report) {
+ qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
}
qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
--
2.20.1
Reviewed-by: Robert Foley <robert.foley@linaro.org>
On Thu, 9 Jul 2020 at 10:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> While there isn't any easy way to make the inline counts thread safe
> we can ensure the callback based ones are. While we are at it we can
> reduce introduce a new option ("idle") to dump a report of the current
> bb and insn count each time a vCPU enters the idle state.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Dave Bort <dbort@dbort.com>
>
> ---
> v2
> - fixup for non-inline linux-user case
> - minor cleanup and re-factor
> ---
> tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
> index df19fd359df3..89c373e19cd8 100644
> --- a/tests/plugin/bb.c
> +++ b/tests/plugin/bb.c
> @@ -16,24 +16,67 @@
>
> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>
> -static uint64_t bb_count;
> -static uint64_t insn_count;
> +typedef struct {
> + GMutex lock;
> + int index;
> + uint64_t bb_count;
> + uint64_t insn_count;
> +} CPUCount;
> +
> +/* Used by the inline & linux-user counts */
> static bool do_inline;
> +static CPUCount inline_count;
> +
> +/* Dump running CPU total on idle? */
> +static bool idle_report;
> +static GPtrArray *counts;
> +static int max_cpus;
> +
> +static void gen_one_cpu_report(CPUCount *count, GString *report)
> +{
> + if (count->bb_count) {
> + g_string_append_printf(report, "CPU%d: "
> + "bb's: %" PRIu64", insns: %" PRIu64 "\n",
> + count->index,
> + count->bb_count, count->insn_count);
> + }
> +}
>
> static void plugin_exit(qemu_plugin_id_t id, void *p)
> {
> - g_autofree gchar *out = g_strdup_printf(
> - "bb's: %" PRIu64", insns: %" PRIu64 "\n",
> - bb_count, insn_count);
> - qemu_plugin_outs(out);
> + g_autoptr(GString) report = g_string_new("");
> +
> + if (do_inline || !max_cpus) {
> + g_string_printf(report, "bb's: %" PRIu64", insns: %" PRIu64 "\n",
> + inline_count.bb_count, inline_count.insn_count);
> + } else {
> + g_ptr_array_foreach(counts, (GFunc) gen_one_cpu_report, report);
> + }
> + qemu_plugin_outs(report->str);
> +}
> +
> +static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
> +{
> + CPUCount *count = g_ptr_array_index(counts, cpu_index);
> + g_autoptr(GString) report = g_string_new("");
> + gen_one_cpu_report(count, report);
> +
> + if (report->len > 0) {
> + g_string_prepend(report, "Idling ");
> + qemu_plugin_outs(report->str);
> + }
> }
>
> static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
> {
> - unsigned long n_insns = (unsigned long)udata;
> + CPUCount *count = max_cpus ?
> + g_ptr_array_index(counts, cpu_index) : &inline_count;
>
> - insn_count += n_insns;
> - bb_count++;
> + unsigned long n_insns = (unsigned long)udata;
> + g_mutex_lock(&count->lock);
> + count->insn_count += n_insns;
> + count->bb_count++;
> + g_mutex_unlock(&count->lock);
> }
>
> static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> @@ -42,9 +85,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>
> if (do_inline) {
> qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
> - &bb_count, 1);
> + &inline_count.bb_count, 1);
> qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
> - &insn_count, n_insns);
> + &inline_count.insn_count, n_insns);
> } else {
> qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
> QEMU_PLUGIN_CB_NO_REGS,
> @@ -56,8 +99,35 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> const qemu_info_t *info,
> int argc, char **argv)
> {
> - if (argc && strcmp(argv[0], "inline") == 0) {
> - do_inline = true;
> + int i;
> +
> + for (i = 0; i < argc; i++) {
> + char *opt = argv[i];
> + if (g_strcmp0(opt, "inline") == 0) {
> + do_inline = true;
> + } else if (g_strcmp0(opt, "idle") == 0) {
> + idle_report = true;
> + } else {
> + fprintf(stderr, "option parsing failed: %s\n", opt);
> + return -1;
> + }
> + }
> +
> + if (info->system_emulation && !do_inline) {
> + max_cpus = info->system.max_vcpus;
> + counts = g_ptr_array_new();
> + for (i = 0; i < max_cpus; i++) {
> + CPUCount *count = g_new0(CPUCount, 1);
> + g_mutex_init(&count->lock);
> + count->index = i;
> + g_ptr_array_add(counts, count);
> + }
> + } else if (!do_inline) {
> + g_mutex_init(&inline_count.lock);
> + }
> +
> + if (idle_report) {
> + qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
> }
>
> qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> --
> 2.20.1
>
On Thu, Jul 09, 2020 at 15:13:22 +0100, Alex Bennée wrote:
> While there isn't any easy way to make the inline counts thread safe
Why not? At least in 64-bit hosts TCG will emit a single write to
update the 64-bit counter.
> we can ensure the callback based ones are. While we are at it we can
> reduce introduce a new option ("idle") to dump a report of the current
s/reduce//
> bb and insn count each time a vCPU enters the idle state.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Dave Bort <dbort@dbort.com>
>
> ---
> v2
> - fixup for non-inline linux-user case
> - minor cleanup and re-factor
> ---
> tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
> index df19fd359df3..89c373e19cd8 100644
> --- a/tests/plugin/bb.c
> +++ b/tests/plugin/bb.c
> @@ -16,24 +16,67 @@
>
> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>
> -static uint64_t bb_count;
> -static uint64_t insn_count;
> +typedef struct {
> + GMutex lock;
> + int index;
> + uint64_t bb_count;
> + uint64_t insn_count;
> +} CPUCount;
Why use a mutex?
Just have a per-vCPU struct that each vCPU thread updates with atomic_write.
Then when we want to print a report we just have to collect the counts
with atomic_read().
Also, consider just adding a comment to bb.c noting that it is not thread-safe,
and having a separate bb-threadsafe.c plugin for patch. The reason is that bb.c is
very simple, which is useful to understand the interface.
Thanks,
E.
Emilio G. Cota <cota@braap.org> writes:
> On Thu, Jul 09, 2020 at 15:13:22 +0100, Alex Bennée wrote:
>> While there isn't any easy way to make the inline counts thread safe
>
> Why not? At least in 64-bit hosts TCG will emit a single write to
> update the 64-bit counter.
Well the operation is an add so that is a load/add/store cycle on
non-x86. If we want to do it properly we should expose an atomic add
operation which may be helper based or maybe generate an atomic
operation if the backend can support it easily.
>
>> we can ensure the callback based ones are. While we are at it we can
>> reduce introduce a new option ("idle") to dump a report of the current
>
> s/reduce//
>
>> bb and insn count each time a vCPU enters the idle state.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Dave Bort <dbort@dbort.com>
>>
>> ---
>> v2
>> - fixup for non-inline linux-user case
>> - minor cleanup and re-factor
>> ---
>> tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 83 insertions(+), 13 deletions(-)
>>
>> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
>> index df19fd359df3..89c373e19cd8 100644
>> --- a/tests/plugin/bb.c
>> +++ b/tests/plugin/bb.c
>> @@ -16,24 +16,67 @@
>>
>> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>
>> -static uint64_t bb_count;
>> -static uint64_t insn_count;
>> +typedef struct {
>> + GMutex lock;
>> + int index;
>> + uint64_t bb_count;
>> + uint64_t insn_count;
>> +} CPUCount;
>
> Why use a mutex?
>
> Just have a per-vCPU struct that each vCPU thread updates with atomic_write.
> Then when we want to print a report we just have to collect the counts
> with atomic_read().
>
> Also, consider just adding a comment to bb.c noting that it is not thread-safe,
> and having a separate bb-threadsafe.c plugin for patch. The reason is that bb.c is
> very simple, which is useful to understand the interface.
>
> Thanks,
> E.
--
Alex Bennée
© 2016 - 2026 Red Hat, Inc.