| Message ID | 20240118032400.3762658-3-pierrick.bouvier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TCG Plugin inline operation enhancement | expand |
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > We introduce a cpu local storage, automatically managed (and extended) > by QEMU itself. Plugin allocate a scoreboard, and don't have to deal > with how many cpus are launched. > > This API will be used by new inline functions but callbacks can benefit > from this as well. This way, they can operate without a global lock for > simple operations. > > New functions: > - qemu_plugin_scoreboard_free > - qemu_plugin_scoreboard_get > - qemu_plugin_scoreboard_new > - qemu_plugin_scoreboard_size > > In more, we define a qemu_plugin_u64_t, which is a simple struct holding > a pointer to a scoreboard, and a given offset. > This allows to have a scoreboard containing structs, without having to > bring offset for all operations on a specific field. > > Since most of the plugins are simply collecting a sum of per-cpu values, > qemu_plugin_u64_t directly support this operation as well. > > New functions: > - qemu_plugin_u64_get > - qemu_plugin_u64_sum > New macros: > - qemu_plugin_u64 > - qemu_plugin_u64_struct > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > --- > include/qemu/plugin.h | 7 +++ > include/qemu/qemu-plugin.h | 75 ++++++++++++++++++++++++++++ > plugins/api.c | 39 +++++++++++++++ > plugins/core.c | 97 ++++++++++++++++++++++++++++++++++++ > plugins/plugin.h | 8 +++ > plugins/qemu-plugins.symbols | 6 +++ > 6 files changed, 232 insertions(+) > > diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h > index 9346249145d..5f340192e56 100644 > --- a/include/qemu/plugin.h > +++ b/include/qemu/plugin.h > @@ -115,6 +115,13 @@ struct qemu_plugin_insn { > bool mem_only; > }; > > +/* A scoreboard is an array of values, indexed by vcpu_index */ > +struct qemu_plugin_scoreboard { > + GArray *data; > + size_t size; Can we get size from GArray->len itself? > + size_t element_size; > +}; > + > /* > * qemu_plugin_insn allocate and cleanup functions. We don't expect to > * cleanup many of these structures. They are reused for each fresh > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index 2c1930e7e45..934059d64c2 100644 > --- a/include/qemu/qemu-plugin.h > +++ b/include/qemu/qemu-plugin.h > @@ -220,6 +220,23 @@ void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id, > struct qemu_plugin_tb; > /** struct qemu_plugin_insn - Opaque handle for a translated instruction */ > struct qemu_plugin_insn; > +/** > + * struct qemu_plugin_scoreboard - Opaque handle for a scoreboard > + * > + * A scoreboard is an array of data, indexed by vcpu_index. > + **/ stray *'s - I think this is what trips up kdoc. > +struct qemu_plugin_scoreboard; > + > +/** > + * qemu_plugin_u64_t - uint64_t member of an entry in a scoreboard We generally reserve lower_case_with_underscores_ending_with_a_t for scalar types. Its also a little generic. Maybe qemu_plugin_scoreboard_ref? > + * > + * This field allows to access a specific uint64_t member in one given entry, > + * located at a specified offset. Inline operations expect this as entry. > + */ > +typedef struct qemu_plugin_u64 { I don't think you need the forward declaration here (which clashes later on). > + struct qemu_plugin_scoreboard *score; > + size_t offset; > +} qemu_plugin_u64_t; > > /** > * enum qemu_plugin_cb_flags - type of callback > @@ -754,5 +771,63 @@ int qemu_plugin_read_register(unsigned int vcpu, > struct qemu_plugin_register *handle, > GByteArray *buf); > > +/** > + * qemu_plugin_scoreboard_new() - alloc a new scoreboard > + * > + * Returns a pointer to a new scoreboard. It must be freed using > + * qemu_plugin_scoreboard_free. > + */ > +QEMU_PLUGIN_API > +struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size); > + > +/** > + * qemu_plugin_scoreboard_free() - free a scoreboard > + * @score: scoreboard to free > + */ > +QEMU_PLUGIN_API > +void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score); > + > +/** > + * qemu_plugin_scoreboard_size() - return size of a scoreboard > + * @score: scoreboard to query > + */ > +QEMU_PLUGIN_API > +size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard > *score); Is this actually host memory size related? The use case in the plugins seems to be a proxy for num_cpus and I'm note sure we want it used that way. We are making the contract that it will always be big enough for the number of vcpus created - maybe that is the helper we need? > + > +/** > + * 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? > + > +/** > + * 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); > +} > + > +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]; } > +} > + > +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); > +} > + > +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? > + 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. > + } > + > + 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 (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; > };
On 1/26/24 19:14, Alex Bennée wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> We introduce a cpu local storage, automatically managed (and extended) >> by QEMU itself. Plugin allocate a scoreboard, and don't have to deal >> with how many cpus are launched. >> >> This API will be used by new inline functions but callbacks can benefit >> from this as well. This way, they can operate without a global lock for >> simple operations. >> >> New functions: >> - qemu_plugin_scoreboard_free >> - qemu_plugin_scoreboard_get >> - qemu_plugin_scoreboard_new >> - qemu_plugin_scoreboard_size >> >> In more, we define a qemu_plugin_u64_t, which is a simple struct holding >> a pointer to a scoreboard, and a given offset. >> This allows to have a scoreboard containing structs, without having to >> bring offset for all operations on a specific field. >> >> Since most of the plugins are simply collecting a sum of per-cpu values, >> qemu_plugin_u64_t directly support this operation as well. >> >> New functions: >> - qemu_plugin_u64_get >> - qemu_plugin_u64_sum >> New macros: >> - qemu_plugin_u64 >> - qemu_plugin_u64_struct >> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> --- >> include/qemu/plugin.h | 7 +++ >> include/qemu/qemu-plugin.h | 75 ++++++++++++++++++++++++++++ >> plugins/api.c | 39 +++++++++++++++ >> plugins/core.c | 97 ++++++++++++++++++++++++++++++++++++ >> plugins/plugin.h | 8 +++ >> plugins/qemu-plugins.symbols | 6 +++ >> 6 files changed, 232 insertions(+) >> >> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h >> index 9346249145d..5f340192e56 100644 >> --- a/include/qemu/plugin.h >> +++ b/include/qemu/plugin.h >> @@ -115,6 +115,13 @@ struct qemu_plugin_insn { >> bool mem_only; >> }; >> >> +/* A scoreboard is an array of values, indexed by vcpu_index */ >> +struct qemu_plugin_scoreboard { >> + GArray *data; >> + size_t size; > > Can we get size from GArray->len itself? > GArray->len matches the allocated size, which is different from "semantic" size. I'll answer to why it was implemented this way in a next comment. >> + size_t element_size; >> +}; >> + >> /* >> * qemu_plugin_insn allocate and cleanup functions. We don't expect to >> * cleanup many of these structures. They are reused for each fresh >> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >> index 2c1930e7e45..934059d64c2 100644 >> --- a/include/qemu/qemu-plugin.h >> +++ b/include/qemu/qemu-plugin.h >> @@ -220,6 +220,23 @@ void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id, >> struct qemu_plugin_tb; >> /** struct qemu_plugin_insn - Opaque handle for a translated instruction */ >> struct qemu_plugin_insn; >> +/** >> + * struct qemu_plugin_scoreboard - Opaque handle for a scoreboard >> + * >> + * A scoreboard is an array of data, indexed by vcpu_index. >> + **/ > > stray *'s - I think this is what trips up kdoc. > Thanks, fixed that in a new version. I now build with documentation enabled, so should not happen again, sorry. >> +struct qemu_plugin_scoreboard; >> + >> +/** >> + * qemu_plugin_u64_t - uint64_t member of an entry in a scoreboard > > We generally reserve lower_case_with_underscores_ending_with_a_t for > scalar types. Its also a little generic. Maybe qemu_plugin_scoreboard_ref? > For _t suffix, I hesitated on this, and followed the qemu_info_t struct example in the same header. In the beginning, I picked qemu_plugin_scoreboard_u64, but realized the name was far too long. qemu_plugin_u64 seemed like the right spot between length/meaning. I would like to defend the u64 meaning, because it was explicitely made to express u64 type, instead of a generic scoreboard entry/ref. In more, based on other comments made, maybe it was not clear that a qemu_plugin_u64 does not necessarily point to a "whole" entry in scoreboard, but it can target a struct member (u64) in a given entry as well. (i.e. scoreboard[i].member). I'll give a more detailed answer below in this message. This way, scoreboard can be composed of struct values, and a qemu_plugin_u64 allows to work on a specific member of that struct without having to manipulate offset everywhere. Thus the get and sum operation on this type. Does it makes more sense to you given this? >> + * >> + * This field allows to access a specific uint64_t member in one given entry, >> + * located at a specified offset. Inline operations expect this as entry. >> + */ >> +typedef struct qemu_plugin_u64 { > > I don't think you need the forward declaration here (which clashes later > on). > Initially wrote this without a typedef, and left this afterwards. Will remove it, thanks! >> + struct qemu_plugin_scoreboard *score; >> + size_t offset; >> +} qemu_plugin_u64_t; >> >> /** >> * enum qemu_plugin_cb_flags - type of callback >> @@ -754,5 +771,63 @@ int qemu_plugin_read_register(unsigned int vcpu, >> struct qemu_plugin_register *handle, >> GByteArray *buf); >> >> +/** >> + * qemu_plugin_scoreboard_new() - alloc a new scoreboard >> + * >> + * Returns a pointer to a new scoreboard. It must be freed using >> + * qemu_plugin_scoreboard_free. >> + */ >> +QEMU_PLUGIN_API >> +struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size); >> + >> +/** >> + * qemu_plugin_scoreboard_free() - free a scoreboard >> + * @score: scoreboard to free >> + */ >> +QEMU_PLUGIN_API >> +void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score); >> + >> +/** >> + * qemu_plugin_scoreboard_size() - return size of a scoreboard >> + * @score: scoreboard to query >> + */ >> +QEMU_PLUGIN_API >> +size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard >> *score); > > Is this actually host memory size related? The use case in the plugins > seems to be a proxy for num_cpus and I'm note sure we want it used that > way. We are making the contract that it will always be big enough for > the number of vcpus created - maybe that is the helper we need? > The current API has a limitation in user mode, where you can't know how many cpus were run overall. The point of scoreboard was to abstract this away, by removing the need for plugins to track this (and use lock, manual memory allocation, and glue code in general). I thought it was more clear to write: for (int i = 0; i < qemu_plugin_scoreboard_size(s); ++i) { void *val = qemu_plugin_scoreboard_get(s, i); } than: for (int i = 0; i < qemu_plugin_max_cpus(s); ++i) { void *val = qemu_plugin_scoreboard_get(s, i); } We can always replace qemu_plugin_scoreboard_size by qemu_plugin_max_cpus (or another name you prefer), which would return the max number of cpus that were used during all the execution, if that works for you. >> + >> +/** >> + * 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!
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > On 1/26/24 19:14, Alex Bennée wrote: >> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >> >>> We introduce a cpu local storage, automatically managed (and extended) >>> by QEMU itself. Plugin allocate a scoreboard, and don't have to deal >>> with how many cpus are launched. >>> >>> This API will be used by new inline functions but callbacks can benefit >>> from this as well. This way, they can operate without a global lock for >>> simple operations. >>> >>> New functions: >>> - qemu_plugin_scoreboard_free >>> - qemu_plugin_scoreboard_get >>> - qemu_plugin_scoreboard_new >>> - qemu_plugin_scoreboard_size >>> >>> In more, we define a qemu_plugin_u64_t, which is a simple struct holding >>> a pointer to a scoreboard, and a given offset. >>> This allows to have a scoreboard containing structs, without having to >>> bring offset for all operations on a specific field. >>> >>> Since most of the plugins are simply collecting a sum of per-cpu values, >>> qemu_plugin_u64_t directly support this operation as well. >>> >>> New functions: >>> - qemu_plugin_u64_get >>> - qemu_plugin_u64_sum >>> New macros: >>> - qemu_plugin_u64 >>> - qemu_plugin_u64_struct >>> >>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> --- >>> include/qemu/plugin.h | 7 +++ >>> include/qemu/qemu-plugin.h | 75 ++++++++++++++++++++++++++++ >>> plugins/api.c | 39 +++++++++++++++ >>> plugins/core.c | 97 ++++++++++++++++++++++++++++++++++++ >>> plugins/plugin.h | 8 +++ >>> plugins/qemu-plugins.symbols | 6 +++ >>> 6 files changed, 232 insertions(+) >>> >>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h >>> index 9346249145d..5f340192e56 100644 >>> --- a/include/qemu/plugin.h >>> +++ b/include/qemu/plugin.h >>> @@ -115,6 +115,13 @@ struct qemu_plugin_insn { >>> bool mem_only; >>> }; >>> +/* A scoreboard is an array of values, indexed by vcpu_index */ >>> +struct qemu_plugin_scoreboard { >>> + GArray *data; >>> + size_t size; >> Can we get size from GArray->len itself? >> > > GArray->len matches the allocated size, which is different from > "semantic" size. I'll answer to why it was implemented this way in a > next comment. Does it? The documentation states: guint len; the number of elements in the GArray not including the possible terminating zero element. I think the actual allocated size is hidden from you by glib as an implementation detail. >>> + size_t element_size; >>> +}; >>> + >>> /* >>> * qemu_plugin_insn allocate and cleanup functions. We don't expect to >>> * cleanup many of these structures. They are reused for each fresh >>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >>> index 2c1930e7e45..934059d64c2 100644 >>> --- a/include/qemu/qemu-plugin.h >>> +++ b/include/qemu/qemu-plugin.h >>> @@ -220,6 +220,23 @@ void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id, >>> struct qemu_plugin_tb; >>> /** struct qemu_plugin_insn - Opaque handle for a translated instruction */ >>> struct qemu_plugin_insn; >>> +/** >>> + * struct qemu_plugin_scoreboard - Opaque handle for a scoreboard >>> + * >>> + * A scoreboard is an array of data, indexed by vcpu_index. >>> + **/ >> stray *'s - I think this is what trips up kdoc. >> > > Thanks, fixed that in a new version. I now build with documentation > enabled, so should not happen again, sorry. > >>> +struct qemu_plugin_scoreboard; >>> + >>> +/** >>> + * qemu_plugin_u64_t - uint64_t member of an entry in a scoreboard >> We generally reserve lower_case_with_underscores_ending_with_a_t for >> scalar types. Its also a little generic. Maybe qemu_plugin_scoreboard_ref? >> > > For _t suffix, I hesitated on this, and followed the qemu_info_t > struct example in the same header. We are nothing if not inconsistent in the code base ;-) There are a number of _t types that in retrospect shouldn't have been. > In the beginning, I picked qemu_plugin_scoreboard_u64, but realized > the name was far too long. qemu_plugin_u64 seemed like the right spot > between length/meaning. > I would like to defend the u64 meaning, because it was explicitely > made to express u64 type, instead of a generic scoreboard entry/ref. > > In more, based on other comments made, maybe it was not clear that a > qemu_plugin_u64 does not necessarily point to a "whole" entry in > scoreboard, but it can target a struct member (u64) in a given entry > as well. (i.e. scoreboard[i].member). I'll give a more detailed answer > below in this message. If we just return values rather than pointers we can stick with simpler types. > This way, scoreboard can be composed of struct values, and a > qemu_plugin_u64 allows to work on a specific member of that struct > without having to manipulate offset everywhere. Thus the get and sum > operation on this type. I would have to see an example of why giving this indirection to the plugin makes things easier. > > Does it makes more sense to you given this? > >>> + * >>> + * This field allows to access a specific uint64_t member in one given entry, >>> + * located at a specified offset. Inline operations expect this as entry. >>> + */ >>> +typedef struct qemu_plugin_u64 { >> > I don't think you need the forward declaration here (which clashes later >> on). >> > > Initially wrote this without a typedef, and left this afterwards. Will > remove it, thanks! > >>> + struct qemu_plugin_scoreboard *score; >>> + size_t offset; >>> +} qemu_plugin_u64_t; >>> /** >>> * enum qemu_plugin_cb_flags - type of callback >>> @@ -754,5 +771,63 @@ int qemu_plugin_read_register(unsigned int vcpu, >>> struct qemu_plugin_register *handle, >>> GByteArray *buf); >>> +/** >>> + * qemu_plugin_scoreboard_new() - alloc a new scoreboard >>> + * >>> + * Returns a pointer to a new scoreboard. It must be freed using >>> + * qemu_plugin_scoreboard_free. >>> + */ >>> +QEMU_PLUGIN_API >>> +struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size); >>> + >>> +/** >>> + * qemu_plugin_scoreboard_free() - free a scoreboard >>> + * @score: scoreboard to free >>> + */ >>> +QEMU_PLUGIN_API >>> +void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score); >>> + >>> +/** >>> + * qemu_plugin_scoreboard_size() - return size of a scoreboard >>> + * @score: scoreboard to query >>> + */ >>> +QEMU_PLUGIN_API >>> +size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard >>> *score); >> Is this actually host memory size related? The use case in the >> plugins >> seems to be a proxy for num_cpus and I'm note sure we want it used that >> way. We are making the contract that it will always be big enough for >> the number of vcpus created - maybe that is the helper we need? >> > > The current API has a limitation in user mode, where you can't know > how many cpus were run overall. The point of scoreboard was to > abstract this away, by removing the need for plugins to track this > (and use lock, manual memory allocation, and glue code in general). Agreed. > I thought it was more clear to write: > for (int i = 0; i < qemu_plugin_scoreboard_size(s); ++i) { > void *val = qemu_plugin_scoreboard_get(s, i); > } > than: > for (int i = 0; i < qemu_plugin_max_cpus(s); ++i) { > void *val = qemu_plugin_scoreboard_get(s, i); > } > > We can always replace qemu_plugin_scoreboard_size by > qemu_plugin_max_cpus (or another name you prefer), which would return > the max number of cpus that were used during all the execution, if > that works for you. I think that works, do we know it even if we haven't allocated any scoreboards? > >>> + >>> +/** >>> + * 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!
On 1/30/24 14:23, Alex Bennée wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> On 1/26/24 19:14, Alex Bennée wrote: >>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >>> >>>> We introduce a cpu local storage, automatically managed (and extended) >>>> by QEMU itself. Plugin allocate a scoreboard, and don't have to deal >>>> with how many cpus are launched. >>>> >>>> This API will be used by new inline functions but callbacks can benefit >>>> from this as well. This way, they can operate without a global lock for >>>> simple operations. >>>> >>>> New functions: >>>> - qemu_plugin_scoreboard_free >>>> - qemu_plugin_scoreboard_get >>>> - qemu_plugin_scoreboard_new >>>> - qemu_plugin_scoreboard_size >>>> >>>> In more, we define a qemu_plugin_u64_t, which is a simple struct holding >>>> a pointer to a scoreboard, and a given offset. >>>> This allows to have a scoreboard containing structs, without having to >>>> bring offset for all operations on a specific field. >>>> >>>> Since most of the plugins are simply collecting a sum of per-cpu values, >>>> qemu_plugin_u64_t directly support this operation as well. >>>> >>>> New functions: >>>> - qemu_plugin_u64_get >>>> - qemu_plugin_u64_sum >>>> New macros: >>>> - qemu_plugin_u64 >>>> - qemu_plugin_u64_struct >>>> >>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>> --- >>>> include/qemu/plugin.h | 7 +++ >>>> include/qemu/qemu-plugin.h | 75 ++++++++++++++++++++++++++++ >>>> plugins/api.c | 39 +++++++++++++++ >>>> plugins/core.c | 97 ++++++++++++++++++++++++++++++++++++ >>>> plugins/plugin.h | 8 +++ >>>> plugins/qemu-plugins.symbols | 6 +++ >>>> 6 files changed, 232 insertions(+) >>>> >>>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h >>>> index 9346249145d..5f340192e56 100644 >>>> --- a/include/qemu/plugin.h >>>> +++ b/include/qemu/plugin.h >>>> @@ -115,6 +115,13 @@ struct qemu_plugin_insn { >>>> bool mem_only; >>>> }; >>>> +/* A scoreboard is an array of values, indexed by vcpu_index */ >>>> +struct qemu_plugin_scoreboard { >>>> + GArray *data; >>>> + size_t size; >>> Can we get size from GArray->len itself? >>> >> >> GArray->len matches the allocated size, which is different from >> "semantic" size. I'll answer to why it was implemented this way in a >> next comment. > > Does it? The documentation states: > > guint len; > > the number of elements in the GArray not including the possible terminating zero element. > > I think the actual allocated size is hidden from you by glib as an > implementation detail. > Sorry, I meant it in the context of a scoreboard where it matches the "allocated" size (considering our exponential growth strategy). Indeed, for the glib, current allocated size is a detail private to implementation. Ideally, this exact function "g_array_maybe_expand" would be useful in a public GArray API, but that's not the case today. https://github.com/GNOME/glib/blob/5f345a265373deff10bd118b7205fb162268eab2/glib/garray.c#L119 So beyond handling both sizes ourselves, I don't see another solution. >>>> + size_t element_size; >>>> +}; >>>> + >>>> /* >>>> * qemu_plugin_insn allocate and cleanup functions. We don't expect to >>>> * cleanup many of these structures. They are reused for each fresh >>>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >>>> index 2c1930e7e45..934059d64c2 100644 >>>> --- a/include/qemu/qemu-plugin.h >>>> +++ b/include/qemu/qemu-plugin.h >>>> @@ -220,6 +220,23 @@ void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id, >>>> struct qemu_plugin_tb; >>>> /** struct qemu_plugin_insn - Opaque handle for a translated instruction */ >>>> struct qemu_plugin_insn; >>>> +/** >>>> + * struct qemu_plugin_scoreboard - Opaque handle for a scoreboard >>>> + * >>>> + * A scoreboard is an array of data, indexed by vcpu_index. >>>> + **/ >>> stray *'s - I think this is what trips up kdoc. >>> >> >> Thanks, fixed that in a new version. I now build with documentation >> enabled, so should not happen again, sorry. >> >>>> +struct qemu_plugin_scoreboard; >>>> + >>>> +/** >>>> + * qemu_plugin_u64_t - uint64_t member of an entry in a scoreboard >>> We generally reserve lower_case_with_underscores_ending_with_a_t for >>> scalar types. Its also a little generic. Maybe qemu_plugin_scoreboard_ref? >>> >> >> For _t suffix, I hesitated on this, and followed the qemu_info_t >> struct example in the same header. > > We are nothing if not inconsistent in the code base ;-) There are a > number of _t types that in retrospect shouldn't have been. > Sure, no problem I'll remove the _t. >> In the beginning, I picked qemu_plugin_scoreboard_u64, but realized >> the name was far too long. qemu_plugin_u64 seemed like the right spot >> between length/meaning. >> I would like to defend the u64 meaning, because it was explicitely >> made to express u64 type, instead of a generic scoreboard entry/ref. >> >> In more, based on other comments made, maybe it was not clear that a >> qemu_plugin_u64 does not necessarily point to a "whole" entry in >> scoreboard, but it can target a struct member (u64) in a given entry >> as well. (i.e. scoreboard[i].member). I'll give a more detailed answer >> below in this message. > > If we just return values rather than pointers we can stick with simpler > types. > Initially, I did this. The problem is that you then need more things when working on a value: - qemu_plugin_u64_set _ probably qemu_plugin_u64_inc(uint64_t val), because qemu_plugin_u64_set(qemu_plugin_u64_get(counter) + 1) is a bit verbose for me. In more, it's inconsistent to have a qemu_plugin_scoreboard_get that return a pointer, while a qemu_plugin_u64_get that return a value. I tried to follow g_array semantic of get equivalent, which gives an address based access instead of a value. >> This way, scoreboard can be composed of struct values, and a >> qemu_plugin_u64 allows to work on a specific member of that struct >> without having to manipulate offset everywhere. Thus the get and sum >> operation on this type. > > I would have to see an example of why giving this indirection to the > plugin makes things easier. > The first version I wrote had simply a scoreboard interface, and quickly, some things appeared as verbose and/or missing. For concrete examples, I would suggest to look at plugins rewritten using this in the series. [08/14] bb.c for instance. In the things that pushed me towards this, if it can help to convince: - having a scoreboard of struct is really useful, and allows to skip allocation/locking problems while manipulating a data more complex than an array of integers. I hope we agree on this. - since qemu_plugin_scoreboard_get has to return a void*, you need to declare a (typed) variable or write a cast every time you just want to access a value. - the sum is still a problem you have to consider, after typing 3 times the same function in plugins, it appears as something that should be part of the interface. Having a sum(ptr, offset, element_size) interface is really error prone. - when registering inline stuff, you need to add a call parameter with offsetof(MyStruct, member), which offers more possibility for a mistake compared to have single place where you define a qemu_plugin_u64_t, and manipulate this everywhere. - register inline op already have a lot of parameters, adding a new "offset" makes it heavy, especially when it's just 0. - possibility to extend this with other types than u64 in the future without going through the "void *" strategy. - favor strongly typed design when possible. Finally, I would add that it's not mandatory to have a global static variable of this type. You can simply register inline op using (qemu_plugin_u64(score)). It's useful when you want to manipulate a precise value, and get its sum over all cpus. >> >> Does it makes more sense to you given this? >> >>>> + * >>>> + * This field allows to access a specific uint64_t member in one given entry, >>>> + * located at a specified offset. Inline operations expect this as entry. >>>> + */ >>>> +typedef struct qemu_plugin_u64 { >>> > I don't think you need the forward declaration here (which clashes later >>> on). >>> >> >> Initially wrote this without a typedef, and left this afterwards. Will >> remove it, thanks! >> >>>> + struct qemu_plugin_scoreboard *score; >>>> + size_t offset; >>>> +} qemu_plugin_u64_t; >>>> /** >>>> * enum qemu_plugin_cb_flags - type of callback >>>> @@ -754,5 +771,63 @@ int qemu_plugin_read_register(unsigned int vcpu, >>>> struct qemu_plugin_register *handle, >>>> GByteArray *buf); >>>> +/** >>>> + * qemu_plugin_scoreboard_new() - alloc a new scoreboard >>>> + * >>>> + * Returns a pointer to a new scoreboard. It must be freed using >>>> + * qemu_plugin_scoreboard_free. >>>> + */ >>>> +QEMU_PLUGIN_API >>>> +struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size); >>>> + >>>> +/** >>>> + * qemu_plugin_scoreboard_free() - free a scoreboard >>>> + * @score: scoreboard to free >>>> + */ >>>> +QEMU_PLUGIN_API >>>> +void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score); >>>> + >>>> +/** >>>> + * qemu_plugin_scoreboard_size() - return size of a scoreboard >>>> + * @score: scoreboard to query >>>> + */ >>>> +QEMU_PLUGIN_API >>>> +size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard >>>> *score); >>> Is this actually host memory size related? The use case in the >>> plugins >>> seems to be a proxy for num_cpus and I'm note sure we want it used that >>> way. We are making the contract that it will always be big enough for >>> the number of vcpus created - maybe that is the helper we need? >>> >> >> The current API has a limitation in user mode, where you can't know >> how many cpus were run overall. The point of scoreboard was to >> abstract this away, by removing the need for plugins to track this >> (and use lock, manual memory allocation, and glue code in general). > > Agreed. > >> I thought it was more clear to write: >> for (int i = 0; i < qemu_plugin_scoreboard_size(s); ++i) { >> void *val = qemu_plugin_scoreboard_get(s, i); >> } >> than: >> for (int i = 0; i < qemu_plugin_max_cpus(s); ++i) { >> void *val = qemu_plugin_scoreboard_get(s, i); >> } >> >> We can always replace qemu_plugin_scoreboard_size by >> qemu_plugin_max_cpus (or another name you prefer), which would return >> the max number of cpus that were used during all the execution, if >> that works for you. > > I think that works, do we know it even if we haven't allocated any > scoreboards? > Excellent point, indeed in this case, you don't have access to this information (scoreboard_size needs a scoreboard in parameter). That would be a good argument to have a max_cpus function instead. Would that name work for you? > >> >>>> + >>>> +/** >>>> + * 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! >
On 1/26/24 19:14, Alex Bennée wrote: >> + 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. > After investigation, current_cpu TLS var is not set in cpus-common.c at this point. Indeed we are not on any cpu_exec path, but in the cpu_realize_fn when calling this (through qemu_plugin_vcpu_init_hook). One obvious fix is to check if it's NULL or not, like: --- a/cpu-common.c +++ b/cpu-common.c @@ -193,7 +193,7 @@ void start_exclusive(void) CPUState *other_cpu; int running_cpus; - if (current_cpu->exclusive_context_count) { + if (current_cpu && current_cpu->exclusive_context_count) { current_cpu->exclusive_context_count++; return; } Does anyone suggest another possible fix? (like define current_cpu somewhere, or moving qemu_plugin_vcpu_init_hook call).
On 1/31/24 11:44, Pierrick Bouvier wrote: > On 1/26/24 19:14, Alex Bennée wrote: >>> + 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. >> > > After investigation, current_cpu TLS var is not set in cpus-common.c at > this point. > > Indeed we are not on any cpu_exec path, but in the cpu_realize_fn when > calling this (through qemu_plugin_vcpu_init_hook). > > One obvious fix is to check if it's NULL or not, like: > --- a/cpu-common.c > +++ b/cpu-common.c > @@ -193,7 +193,7 @@ void start_exclusive(void) > CPUState *other_cpu; > int running_cpus; > > - if (current_cpu->exclusive_context_count) { > + if (current_cpu && current_cpu->exclusive_context_count) { > current_cpu->exclusive_context_count++; > return; > } > > Does anyone suggest another possible fix? (like define current_cpu > somewhere, or moving qemu_plugin_vcpu_init_hook call). Running init_hook asynchronously on cpu works and solves the problem, without any need to modify start/end exclusive code.
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h index 9346249145d..5f340192e56 100644 --- a/include/qemu/plugin.h +++ b/include/qemu/plugin.h @@ -115,6 +115,13 @@ struct qemu_plugin_insn { bool mem_only; }; +/* A scoreboard is an array of values, indexed by vcpu_index */ +struct qemu_plugin_scoreboard { + GArray *data; + size_t size; + size_t element_size; +}; + /* * qemu_plugin_insn allocate and cleanup functions. We don't expect to * cleanup many of these structures. They are reused for each fresh diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index 2c1930e7e45..934059d64c2 100644 --- a/include/qemu/qemu-plugin.h +++ b/include/qemu/qemu-plugin.h @@ -220,6 +220,23 @@ void qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id, struct qemu_plugin_tb; /** struct qemu_plugin_insn - Opaque handle for a translated instruction */ struct qemu_plugin_insn; +/** + * struct qemu_plugin_scoreboard - Opaque handle for a scoreboard + * + * A scoreboard is an array of data, indexed by vcpu_index. + **/ +struct qemu_plugin_scoreboard; + +/** + * qemu_plugin_u64_t - uint64_t member of an entry in a scoreboard + * + * This field allows to access a specific uint64_t member in one given entry, + * located at a specified offset. Inline operations expect this as entry. + */ +typedef struct qemu_plugin_u64 { + struct qemu_plugin_scoreboard *score; + size_t offset; +} qemu_plugin_u64_t; /** * enum qemu_plugin_cb_flags - type of callback @@ -754,5 +771,63 @@ int qemu_plugin_read_register(unsigned int vcpu, struct qemu_plugin_register *handle, GByteArray *buf); +/** + * qemu_plugin_scoreboard_new() - alloc a new scoreboard + * + * Returns a pointer to a new scoreboard. It must be freed using + * qemu_plugin_scoreboard_free. + */ +QEMU_PLUGIN_API +struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size); + +/** + * qemu_plugin_scoreboard_free() - free a scoreboard + * @score: scoreboard to free + */ +QEMU_PLUGIN_API +void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score); + +/** + * qemu_plugin_scoreboard_size() - return size of a scoreboard + * @score: scoreboard to query + */ +QEMU_PLUGIN_API +size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard *score); + +/** + * 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_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; +} + +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; +} + +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); +} + +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; +} + +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; + 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 + } + + 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); + + 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; };
We introduce a cpu local storage, automatically managed (and extended) by QEMU itself. Plugin allocate a scoreboard, and don't have to deal with how many cpus are launched. This API will be used by new inline functions but callbacks can benefit from this as well. This way, they can operate without a global lock for simple operations. New functions: - qemu_plugin_scoreboard_free - qemu_plugin_scoreboard_get - qemu_plugin_scoreboard_new - qemu_plugin_scoreboard_size In more, we define a qemu_plugin_u64_t, which is a simple struct holding a pointer to a scoreboard, and a given offset. This allows to have a scoreboard containing structs, without having to bring offset for all operations on a specific field. Since most of the plugins are simply collecting a sum of per-cpu values, qemu_plugin_u64_t directly support this operation as well. New functions: - qemu_plugin_u64_get - qemu_plugin_u64_sum New macros: - qemu_plugin_u64 - qemu_plugin_u64_struct Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- include/qemu/plugin.h | 7 +++ include/qemu/qemu-plugin.h | 75 ++++++++++++++++++++++++++++ plugins/api.c | 39 +++++++++++++++ plugins/core.c | 97 ++++++++++++++++++++++++++++++++++++ plugins/plugin.h | 8 +++ plugins/qemu-plugins.symbols | 6 +++ 6 files changed, 232 insertions(+)