Re: [PATCH v3 05/17] plugins: scoreboard API

[Richard Henderson <richard.henderson@linaro.org>]

On 2/6/24 19:24, Pierrick Bouvier wrote:
> 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.
> 
> At any point during execution, any scoreboard will be dimensioned with
> at least qemu_plugin_num_vcpus entries.
> 
> New functions:
> - qemu_plugin_scoreboard_find
> - qemu_plugin_scoreboard_free
> - qemu_plugin_scoreboard_new
> 
> In more, we define a qemu_plugin_u64, 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 directly support this operation as well.
> 
> New functions:
> - qemu_plugin_u64_add
> - qemu_plugin_u64_get
> - qemu_plugin_u64_set
> - qemu_plugin_u64_sum
> New macros:
> - qemu_plugin_scoreboard_u64
> - qemu_plugin_scoreboard_u64_in_struct

I think the u64 stuff should be a second patch built upon the basic scoreboard support.

> +/* A scoreboard is an array of values, indexed by vcpu_index */
> +struct qemu_plugin_scoreboard {
> +    GArray *data;
> +};

Unnecessary?  Generates an extra pointer dereference for no apparent benefit. 
Alternately, might be useful for other data structure changes...

> +/**
> + * typedef qemu_plugin_u64 - 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 {
> +    struct qemu_plugin_scoreboard *score;

Embed the struct instead?

> @@ -31,6 +31,9 @@ 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;

Why a hash table?  All you want is to be able to iterate through all, and add/remove 
easily.  Seems like QLIST from <qemu/queue.h> would be better, and the QLIST_ENTRY member 
would make struct qemu_plugin_scoreboard useful.


r~