[dpdk-dev] [RFC 1/2] telemetry: support some recursive data objects

[Bruce Richardson]

On Fri, Jun 12, 2020 at 11:53:43AM +0100, Ciara Power wrote:
> Dict data objects now support uint64_t array data object values.
> Only one level of recursion supported.
> 
> Signed-off-by: Ciara Power <ciara.power at intel.com>
> ---
>  lib/librte_telemetry/rte_telemetry.h          | 27 +++++++++++++++
>  .../rte_telemetry_version.map                 |  2 ++
>  lib/librte_telemetry/telemetry.c              | 34 +++++++++++++++++++
>  lib/librte_telemetry/telemetry_data.c         | 18 ++++++++++
>  lib/librte_telemetry/telemetry_data.h         |  3 ++
>  lib/librte_telemetry/telemetry_json.h         | 17 ++++++++++
>  6 files changed, 101 insertions(+)
> 
> diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h
> index 2c3c96cf7..dc18c34d0 100644
> --- a/lib/librte_telemetry/rte_telemetry.h
> +++ b/lib/librte_telemetry/rte_telemetry.h
> @@ -44,6 +44,7 @@ enum rte_tel_value_type {
>  	RTE_TEL_STRING_VAL, /** a string value */
>  	RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
>  	RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
> +	RTE_TEL_DATA_VAL,   /** a rte_tel_data pointer value */
>  };
>  
>  /**
> @@ -188,6 +189,22 @@ int
>  rte_tel_data_add_dict_u64(struct rte_tel_data *d,
>  		const char *name, uint64_t val);
>  
> +/**
> + * Add a data object pointer to a dictionary.
> + * The dict must have been started by rte_tel_data_start_dict().
> + *
> + * @param d
> + *   The data structure passed to the callback
> + * @param x
> + *   The data pointer to be returned in the dictionary
> + * @return
> + *   0 on success, negative errno on error
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_dict_data(struct rte_tel_data *d, const char *name,
> +		struct rte_tel_data *val);
> +
>  /**
>   * This telemetry callback is used when registering a telemetry command.
>   * It handles getting and formatting information to be returned to telemetry
> @@ -253,4 +270,14 @@ int
>  rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
>  		const char **err_str);
>  
> +/**
> + * Get the size of the rte_tel_data struct.
> + *
> + * @return
> + *  size_t of the struct
> + */
> +__rte_experimental
> +size_t
> +rte_tel_get_data_size(void);
> +
Thanks for this work. Thinking about this, the biggest issue I believe is
always going to be the memory management aspect of it. It seems here that
you are providing an API to allow the memory allocation of the additional
object to be done by the callback itself, but that leaves open a problem of
freeing the memory again - how does the telemetry library know what
allocation function was used, so it knows to free it appropriately? This
could be done by function pointer passed in by when adding the data
element, but I think that is clunky.

Perhaps a better approach here might be to instead add APIs for data alloc
and free, so that there is only one way to manage the memory. That would
remove the need for this size function.