+
+/**
+ * qemu_plugin_scoreboard_get() - access value from a scoreboard
+ * @score: scoreboard to query
+ * @vcpu_index: entry index
+ *
+ * Returns address of entry of a scoreboard matching a given vcpu_index. This
+ * address can be modified later if scoreboard is resized.
+ */
+QEMU_PLUGIN_API
+void *qemu_plugin_scoreboard_get(struct qemu_plugin_scoreboard *score,
+ unsigned int vcpu_index);
+
+/* Macros to define a qemu_plugin_u64_t */
+#define qemu_plugin_u64(score) \
+ (qemu_plugin_u64_t){score, 0}
+#define qemu_plugin_u64_struct(score, type, member) \
+ (qemu_plugin_u64_t){score, offsetof(type, member)}
qemu_plugin_val_ref and qemu_plugin_struct_ref?
Same comment as before, it would be more clear to keep the u64 in the
name if possible, so we know which data type we manipulate.
+
+/**
+ * qemu_plugin_u64_get() - access specific uint64_t in a scoreboard
+ * @entry: entry to query
+ * @vcpu_index: entry index
+ *
+ * Returns address of a specific member in a scoreboard entry, matching a given
+ * vcpu_index.
+ */
+QEMU_PLUGIN_API
+uint64_t *qemu_plugin_u64_get(qemu_plugin_u64_t entry, unsigned int
vcpu_index);
+
+/**
+ * qemu_plugin_u64_sum() - return sum of all values in a scoreboard
+ * @entry: entry to sum
+ */
+QEMU_PLUGIN_API
+uint64_t qemu_plugin_u64_sum(qemu_plugin_u64_t entry);
#endif /* QEMU_QEMU_PLUGIN_H */
diff --git a/plugins/api.c b/plugins/api.c
index e777eb4e9fc..4de94e798c6 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -547,3 +547,42 @@ static void __attribute__((__constructor__))
qemu_api_init(void)
qemu_mutex_init(®_handle_lock);
}
+
+struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
+{
+ return plugin_scoreboard_new(element_size);
+}
+
+void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score)
+{
+ plugin_scoreboard_free(score);
+}
+
+size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard *score)
+{
+ return score->size;
So this would be score->data->len
+}
+
+void *qemu_plugin_scoreboard_get(struct qemu_plugin_scoreboard *score,
+ unsigned int vcpu_index)
+{
+ g_assert(vcpu_index < qemu_plugin_scoreboard_size(score));
+ char *ptr = score->data->data;
+ return ptr + vcpu_index * score->element_size;
GArray has accesses functions for this:
return &g_array_index(score->data,
g_array_get_element_size(score->data),
vcpu_index);
Yes, I'll update this.
+}
+
+uint64_t *qemu_plugin_u64_get(qemu_plugin_u64_t entry,
+ unsigned int vcpu_index)
+{
+ char *ptr = (char *) qemu_plugin_scoreboard_get(entry.score, vcpu_index);
+ return (uint64_t *)(ptr + entry.offset);
Not sure whats going with the casting here but I wonder if we are
giving
the user too much rope. Why not to return the value itself? In fact why
do we treat things as an offset rather than an index of uint64_t.
The qemu_plugin_scoreboard_get can return uint64_t * and the
individual
get becomes:
uint64_t *qemu_plugin_scoreboard_get(struct
qemu_plugin_scoreboard *score,
unsigned int vcpu_index)
{
g_assert(vcpu_index < qemu_plugin_scoreboard_size(score));
return &g_array_index(score->data, score->element_size, vcpu_index);
}
uint64_t qemu_plugin_u64_get(struct qemu_plugin_scoreboard
*score,
unsigned int vcpu_index, unsigned int
score_index)
{
g_assert(score_index < score->num_elements);
uint64_t *sb = qemu_plugin_scoreboard_get(score, vcpu_index);
return sb[score_index];
}
As said above, maybe it was not clear in the commit (though expressed
in commit message) what was a distinction between a scoreboard, and a
plugin_u64.
The latter is an abstraction of a specific member in one scoreboard
entry, while a scoreboard holds any type.
Let me write a concrete example, which will be clearer:
typedef struct {
char data1;
bool data2;
void* data3;
enum ...;
uint64_t counter;
} CPUData;
score = qemu_plugin_scoreboard_new(sizeof(CPUData));
qemu_plugin_u64_t counter =
qemu_plugin_u64_struct(score, CPUData, counter));
From there:
qemu_plugin_u64_get(counter, 2) returns &score[2].counter.
qemu_plugin_u64_sum(counter) returns
sum(i: [0:max_cpus[, score[i].counter).
With this, a plugin can simply have an abstract representation of cpu
local value, than can be get or set (since we return a pointer with
get) and easily summed, which is what most of the plugins want to do
in the end (at least from what I observed in existing ones).
If we stick to a void* scoreboard, we end up writing the sum function
in every plugin. In more, every API function "inline" operating on a
scoreboard would need the offsetof(CPUData, counter) passed in
parameter, which is error prone and repetitive.
With this explaination, do you see more the value to have a scoreboard
*and* a plugin_u64 struct to access it?
+}
+
+uint64_t qemu_plugin_u64_sum(qemu_plugin_u64_t entry)
+{
+ uint64_t total = 0;
+ for (int i = 0; i < qemu_plugin_scoreboard_size(entry.score); ++i) {
+ total += *qemu_plugin_u64_get(entry, i);
+ }
+ return total;
+}
diff --git a/plugins/core.c b/plugins/core.c
index e07b9cdf229..0286a127810 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -209,6 +209,71 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum
qemu_plugin_event ev,
do_plugin_register_cb(id, ev, func, udata);
}
+struct resize_scoreboard_args {
+ size_t new_alloc_size;
+ size_t new_size;
+};
+
+static void plugin_resize_one_scoreboard(gpointer key,
+ gpointer value,
+ gpointer user_data)
+{
+ struct qemu_plugin_scoreboard *score =
+ (struct qemu_plugin_scoreboard *) value;
+ struct resize_scoreboard_args *args =
+ (struct resize_scoreboard_args *) user_data;
+ if (score->data->len != args->new_alloc_size) {
+ g_array_set_size(score->data, args->new_alloc_size);
+ }
+ score->size = args->new_size;
I think you have this in len so if you skip the extra field you can
just
do:
new_size = GPOINTER_TO_UINT(user_data);
If we can avoid managing allocated vs real size, yes.
+}
+
+static void plugin_grow_scoreboards__locked(CPUState *cpu)
+{
+ if (cpu->cpu_index < plugin.scoreboard_size) {
+ return;
+ }
+
+ bool need_realloc = FALSE;
+ while (cpu->cpu_index >= plugin.scoreboard_alloc_size) {
+ plugin.scoreboard_alloc_size *= 2;
I suspect for USER this makes sense, if we every end up doing system
expansion then it would just be +1 at a time. In fact given glib does:
gsize want_alloc = g_nearest_pow (g_array_elt_len (array,
want_len));
maybe we just do a simple increment and leave it up glib to handle
the
exponential growth?
Initially, I wanted to go this way, but it seems like glib does not
offer alloc_size information about a GArray. I looked for this in
documentation and doc, and maybe missed it.
If you have an idea to detect this, I would love to just use it, and
it would made the code much more simple, avoiding the need to keep
track of two sizes.
The problem we try to solve is to detect when the pointer will be
reallocated, with the consequence that all tb must be flushed and
retranslated to use the new one instead.
+ need_realloc = TRUE;
+ }
+ plugin.scoreboard_size = cpu->cpu_index + 1;
+ g_assert(plugin.scoreboard_size <= plugin.scoreboard_alloc_size);
+
+ if (g_hash_table_size(plugin.scoreboards) == 0) {
+ /* nothing to do, we just updated sizes for future scoreboards */
+ return;
+ }
+
+ if (need_realloc) {
+#ifdef CONFIG_USER_ONLY
+ /**
+ * cpus must be stopped, as some tb might still use an existing
+ * scoreboard.
+ */
+ start_exclusive();
+#endif
Hmm this seems wrong to be USER_ONLY. While we don't expect to
resize in
system mode if we did we certainly want to do it during exclusive
periods.
I agree. Initially did this for both (system and user), but in system,
we hit a segfault.
$ ./build/qemu-system-x86_64 -plugin ./build/tests/plugin/libinline.so
-smp 17 # > 16 will trigger a reallocation of scoreboard
../cpu-common.c:196:20: runtime error: member access within null
pointer of type 'struct CPUState'
It seems like something is not set yet at this time (current_cpu TLS
var), which result in this problem. Maybe this code simply reveals an
existing problem, what do you think?
+ }
+
+ struct resize_scoreboard_args args = {
+ .new_alloc_size = plugin.scoreboard_alloc_size,
+ .new_size = plugin.scoreboard_size
+ };
+ g_hash_table_foreach(plugin.scoreboards,
+ &plugin_resize_one_scoreboard,
+ &args);
This can just be g_hash_table_foreach(plugin.scoreboards,
&plugin_resize_one_scoreboard,
GUINT_TO_POINTER(plugin.scoreboard_alloc_size))
If we use a single size, yes.
+
+ if (need_realloc) {
+ /* force all tb to be flushed, as scoreboard pointers were changed. */
+ tb_flush(cpu);
+#ifdef CONFIG_USER_ONLY
+ end_exclusive();
+#endif
+ }
+}
+
void qemu_plugin_vcpu_init_hook(CPUState *cpu)
{
bool success;
@@ -218,6 +283,7 @@ void qemu_plugin_vcpu_init_hook(CPUState *cpu)
success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index,
&cpu->cpu_index);
g_assert(success);
+ plugin_grow_scoreboards__locked(cpu);
qemu_rec_mutex_unlock(&plugin.lock);
plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT);
@@ -576,8 +642,39 @@ static void __attribute__((__constructor__))
plugin_init(void)
qemu_rec_mutex_init(&plugin.lock);
plugin.id_ht = g_hash_table_new(g_int64_hash, g_int64_equal);
plugin.cpu_ht = g_hash_table_new(g_int_hash, g_int_equal);
+ plugin.scoreboards = g_hash_table_new(g_int64_hash, g_int64_equal);
+ plugin.scoreboard_size = 1;
+ plugin.scoreboard_alloc_size = 16; /* avoid frequent reallocation */
QTAILQ_INIT(&plugin.ctxs);
qht_init(&plugin.dyn_cb_arr_ht, plugin_dyn_cb_arr_cmp, 16,
QHT_MODE_AUTO_RESIZE);
atexit(qemu_plugin_atexit_cb);
}
+
+struct qemu_plugin_scoreboard *plugin_scoreboard_new(size_t element_size)
+{
+ struct qemu_plugin_scoreboard *score =
+ g_malloc0(sizeof(struct qemu_plugin_scoreboard));
+ score->data = g_array_new(FALSE, TRUE, element_size);
+ g_array_set_size(score->data, plugin.scoreboard_alloc_size);
+ score->size = plugin.scoreboard_size;
+ score->element_size = element_size;
+
+ qemu_rec_mutex_lock(&plugin.lock);
+ bool inserted = g_hash_table_insert(plugin.scoreboards, score, score);
+ g_assert(inserted);
+ qemu_rec_mutex_unlock(&plugin.lock);
+
+ return score;
+}
+
+void plugin_scoreboard_free(struct qemu_plugin_scoreboard *score)
+{
+ qemu_rec_mutex_lock(&plugin.lock);
+ bool removed = g_hash_table_remove(plugin.scoreboards, score);
+ g_assert(removed);
+ qemu_rec_mutex_unlock(&plugin.lock);
+
+ g_array_free(score->data, TRUE);
+ g_free(score);
+}
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 2c278379b70..99829c40886 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -31,6 +31,10 @@ struct qemu_plugin_state {
* but with the HT we avoid adding a field to CPUState.
*/
GHashTable *cpu_ht;
+ /* Scoreboards, indexed by their addresses. */
+ GHashTable *scoreboards;
+ size_t scoreboard_size;
+ size_t scoreboard_alloc_size;
DECLARE_BITMAP(mask, QEMU_PLUGIN_EV_MAX);
/*
* @lock protects the struct as well as ctx->uninstalling.
@@ -99,4 +103,8 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int
cpu_index);
+struct qemu_plugin_scoreboard *plugin_scoreboard_new(size_t
element_size);
+
+void plugin_scoreboard_free(struct qemu_plugin_scoreboard *score);
+
#endif /* PLUGIN_H */
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 6963585c1ea..93866d1b5f2 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -38,10 +38,16 @@
qemu_plugin_register_vcpu_tb_exec_inline;
qemu_plugin_register_vcpu_tb_trans_cb;
qemu_plugin_reset;
+ qemu_plugin_scoreboard_free;
+ qemu_plugin_scoreboard_get;
+ qemu_plugin_scoreboard_new;
+ qemu_plugin_scoreboard_size;
qemu_plugin_start_code;
qemu_plugin_tb_get_insn;
qemu_plugin_tb_n_insns;
qemu_plugin_tb_vaddr;
+ qemu_plugin_u64_get;
+ qemu_plugin_u64_sum;
qemu_plugin_uninstall;
qemu_plugin_vcpu_for_each;
};
Thanks for your complete and detailed review!