From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id 4FE72A0350;
Wed, 24 Jun 2020 17:18:13 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id B519A1DA0A;
Wed, 24 Jun 2020 17:18:12 +0200 (CEST)
Received: from mga17.intel.com (mga17.intel.com [192.55.52.151])
by dpdk.org (Postfix) with ESMTP id BC6C61D9FA
for <dev@dpdk.org>; Wed, 24 Jun 2020 17:18:10 +0200 (CEST)
IronPort-SDR: o1bcR9rA5zhR82wSzkv++NKKwutrfOgZ9mSzVQbL7vU4AfonsXJ26alVLU5gJSP5FLeprWRvwi
g7P1phQ3hwSw==
X-IronPort-AV: E=McAfee;i="6000,8403,9662"; a="124761155"
X-IronPort-AV: E=Sophos;i="5.75,275,1589266800"; d="scan'208";a="124761155"
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga004.jf.intel.com ([10.7.209.38])
by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
24 Jun 2020 08:18:09 -0700
IronPort-SDR: /jDbwkkGL85BolB6+DSgzyrqRXc7+ciB5KResdViTlDQJtP0ntyGgzgTStFnxsKhsKGw+WYBxi
OLmp6x1P2U8Q==
X-IronPort-AV: E=Sophos;i="5.75,275,1589266800"; d="scan'208";a="423402652"
Received: from bricha3-mobl.ger.corp.intel.com ([10.252.2.40])
by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA;
24 Jun 2020 08:18:07 -0700
Date: Wed, 24 Jun 2020 16:18:04 +0100
From: Bruce Richardson <bruce.richardson@intel.com>
To: Ciara Power <ciara.power@intel.com>
Cc: kevin.laatz@intel.com, thomas@monjalon.net, ferruh.yigit@intel.com,
arybchenko@solarflare.com, dev@dpdk.org, keith.wiles@intel.com
Message-ID: <20200624151804.GB1132@bricha3-MOBL.ger.corp.intel.com>
References: <20200612105344.15383-1-ciara.power@intel.com>
<20200624134824.16210-1-ciara.power@intel.com>
<20200624134824.16210-2-ciara.power@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200624134824.16210-2-ciara.power@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] telemetry: support array values in
data objects
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On Wed, Jun 24, 2020 at 02:48:23PM +0100, Ciara Power wrote:
> Arrays and Dicts now support uint64_t, int and string
> array values. Only one level of recursion supported.
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
>
A number of comments inline below. Once fixed you can include my ack on V3.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> v2:
> - Added support for arrays to have container values.
> - Added support for int and string arrays within dict/array.
> - Added APIs for internal container memory management.
> ---
> lib/librte_telemetry/rte_telemetry.h | 67 +++++++++++++++++++
> .../rte_telemetry_version.map | 4 ++
> lib/librte_telemetry/telemetry.c | 56 ++++++++++++++++
> lib/librte_telemetry/telemetry_data.c | 51 ++++++++++++++
> lib/librte_telemetry/telemetry_data.h | 7 ++
> lib/librte_telemetry/telemetry_json.h | 33 +++++++++
> 6 files changed, 218 insertions(+)
>
> diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h
> index eb7f2c917..89aff3429 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_CONTAINER, /** a container struct */
> };
>
> /**
> @@ -134,6 +135,28 @@ __rte_experimental
> int
> rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x);
>
> +/**
> + * Add a container to an array.
I think you need to explain the term container a bit, as in "an existing
telemetry data array", or similar.
> + * The array must have been started by rte_tel_data_start_array() with
> + * RTE_TEL_CONTAINER as the type parameter. The rte_tel_data type
> + * must be an array of type u64/int/string.
> + *
> + * @param d
> + * The data structure passed to the callback
> + * @param val
> + * The rte_tel_data pointer to be returned in the container.
"The pointer to the container to be stored in the array"
> + * @param keep
> + * Integer value to represent if rte_tel_data memory should be freed
> + * after use.
This would be clearer as a boolean, or call it a flag. How about:
"Flag to indicate that the container memory should not be automatically
freed by the telemetry library once it has finished with the data"
> + * 1 = keep, 0 = free.
> + * @return
> + * 0 on success, negative errno on error
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_array_container(struct rte_tel_data *d,
> + struct rte_tel_data *val, int keep);
> +
> /**
> * Add a string value to a dictionary.
> * The dict must have been started by rte_tel_data_start_dict().
> @@ -188,6 +211,27 @@ int
> rte_tel_data_add_dict_u64(struct rte_tel_data *d,
> const char *name, uint64_t val);
>
> +/**
> + * Add a container to a dictionary.
> + * The dict must have been started by rte_tel_data_start_dict().
> + * The rte_tel_data type must be an array of type u64/int/string.
> + *
> + * @param d
> + * The data structure passed to the callback
> + * @param val
> + * The rte_tel_data pointer to be returned in the container.
> + * @param keep
> + * Integer value to represent if rte_tel_data memory should be freed
> + * after use.
> + * 1 = keep, 0 = free.
> + * @return
> + * 0 on success, negative errno on error
> + */
Similar comments to the above here.
> +__rte_experimental
> +int
> +rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> + struct rte_tel_data *val, int keep);
> +
> /**
> * This telemetry callback is used when registering a telemetry command.
> * It handles getting and formatting information to be returned to telemetry
> @@ -262,4 +306,27 @@ int
> rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
> const char **err_str);
>
> +/**
> + * @internal
> + * Get a data object with memory allocated.
> + *
Do we want to use the term container here also?
> + * @return
> + * Pointer to rte_tel_data.
> + */
> +__rte_experimental
> +struct rte_tel_data *
> +rte_tel_data_alloc(void);
> +
> +/**
> + * @internal
> + * Free a data object that has memory allocated.
> + *
> + * @param data
> + * Pointer to rte_tel_data.
> + *.
> + */
> +__rte_experimental
> +void
> +rte_tel_data_free(struct rte_tel_data *data);
> +
> #endif
> diff --git a/lib/librte_telemetry/rte_telemetry_version.map b/lib/librte_telemetry/rte_telemetry_version.map
> index 86433c21d..d1dbf8d58 100644
> --- a/lib/librte_telemetry/rte_telemetry_version.map
> +++ b/lib/librte_telemetry/rte_telemetry_version.map
> @@ -1,12 +1,16 @@
> EXPERIMENTAL {
> global:
>
> + rte_tel_data_add_array_container;
> rte_tel_data_add_array_int;
> rte_tel_data_add_array_string;
> rte_tel_data_add_array_u64;
> + rte_tel_data_add_dict_container;
> rte_tel_data_add_dict_int;
> rte_tel_data_add_dict_string;
> rte_tel_data_add_dict_u64;
> + rte_tel_data_alloc;
> + rte_tel_data_free;
> rte_tel_data_start_array;
> rte_tel_data_start_dict;
> rte_tel_data_string;
> diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c
> index e7e3d861d..a36f60a6c 100644
> --- a/lib/librte_telemetry/telemetry.c
> +++ b/lib/librte_telemetry/telemetry.c
> @@ -120,6 +120,35 @@ command_help(const char *cmd __rte_unused, const char *params,
> return 0;
> }
>
> +static int
> +recursive_data_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
Not sure recursive in the name is great. Do we want to call this
"container_to_json" or "array_to_json" perhaps?
> +{
> + size_t used = 0;
> + unsigned int i;
> +
> + if (d->type != RTE_TEL_ARRAY_U64 && d->type != RTE_TEL_ARRAY_INT
> + && d->type != RTE_TEL_ARRAY_STRING)
> + return snprintf(out_buf, buf_len, "null");
> +
> + used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> + if (d->type == RTE_TEL_ARRAY_U64)
> + for (i = 0; i < d->data_len; i++)
> + used = rte_tel_json_add_array_u64(out_buf,
> + buf_len, used,
> + d->data.array[i].u64val);
> + if (d->type == RTE_TEL_ARRAY_INT)
> + for (i = 0; i < d->data_len; i++)
> + used = rte_tel_json_add_array_int(out_buf,
> + buf_len, used,
> + d->data.array[i].ival);
> + if (d->type == RTE_TEL_ARRAY_STRING)
> + for (i = 0; i < d->data_len; i++)
> + used = rte_tel_json_add_array_string(out_buf,
> + buf_len, used,
> + d->data.array[i].sval);
> + return used;
> +}
> +
> static void
> output_json(const char *cmd, const struct rte_tel_data *d, int s)
> {
> @@ -166,6 +195,20 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> buf_len, used,
> v->name, v->value.u64val);
> break;
> + case RTE_TEL_CONTAINER:
> + {
> + char temp[buf_len];
> + const struct container *cont =
> + &v->value.container;
> + if (recursive_data_json(cont->data,
> + temp, buf_len) != 0)
> + used = rte_tel_json_add_obj_json(
> + cb_data_buf,
> + buf_len, used,
> + v->name, temp);
> + if (!cont->keep)
> + rte_tel_data_free(cont->data);
> + }
> }
> }
> used += prefix_used;
> @@ -174,6 +217,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> case RTE_TEL_ARRAY_STRING:
> case RTE_TEL_ARRAY_INT:
> case RTE_TEL_ARRAY_U64:
> + case RTE_TEL_ARRAY_CONTAINER:
> prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> MAX_CMD_LEN, cmd);
> cb_data_buf = &out_buf[prefix_used];
> @@ -194,6 +238,18 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> used = rte_tel_json_add_array_u64(cb_data_buf,
> buf_len, used,
> d->data.array[i].u64val);
> + else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
> + char temp[buf_len];
> + const struct container *rec_data =
> + &d->data.array[i].container;
> + if (recursive_data_json(rec_data->data,
> + temp, buf_len) != 0)
> + used = rte_tel_json_add_array_json(
> + cb_data_buf,
> + buf_len, used, temp);
> + if (!rec_data->keep)
> + rte_tel_data_free(rec_data->data);
> + }
> used += prefix_used;
> used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
> break;
> diff --git a/lib/librte_telemetry/telemetry_data.c b/lib/librte_telemetry/telemetry_data.c
> index f424bbd48..77b0fe09a 100644
> --- a/lib/librte_telemetry/telemetry_data.c
> +++ b/lib/librte_telemetry/telemetry_data.c
> @@ -14,6 +14,7 @@ rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
> RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> RTE_TEL_ARRAY_INT, /* RTE_TEL_INT_VAL = 1 */
> RTE_TEL_ARRAY_U64, /* RTE_TEL_u64_VAL = 2 */
> + RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> };
> d->type = array_types[type];
> d->data_len = 0;
> @@ -74,6 +75,23 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
> return 0;
> }
>
> +int
> +rte_tel_data_add_array_container(struct rte_tel_data *d,
> + struct rte_tel_data *val, int keep)
> +{
> + if (d->type != RTE_TEL_ARRAY_CONTAINER ||
> + (val->type != RTE_TEL_ARRAY_U64
> + && val->type != RTE_TEL_ARRAY_INT
> + && val->type != RTE_TEL_ARRAY_STRING))
> + return -EINVAL;
> + if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> + return -ENOSPC;
> +
> + d->data.array[d->data_len].container.data = val;
> + d->data.array[d->data_len++].container.keep = !!keep;
> + return 0;
> +}
> +
> int
> rte_tel_data_add_dict_string(struct rte_tel_data *d, const char *name,
> const char *val)
> @@ -128,3 +146,36 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
> const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> }
> +
> +int
> +rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> + struct rte_tel_data *val, int keep)
> +{
> + struct tel_dict_entry *e = &d->data.dict[d->data_len];
> +
> + if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
> + && val->type != RTE_TEL_ARRAY_INT
> + && val->type != RTE_TEL_ARRAY_STRING))
> + return -EINVAL;
> + if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> + return -ENOSPC;
> +
> + d->data_len++;
> + e->type = RTE_TEL_CONTAINER;
> + e->value.container.data = val;
> + e->value.container.keep = !!keep;
> + const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> + return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> +}
> +
> +struct rte_tel_data *
> +rte_tel_data_alloc(void)
> +{
> + return malloc(sizeof(struct rte_tel_data));
> +}
> +
> +void
> +rte_tel_data_free(struct rte_tel_data *data)
> +{
> + free(data);
> +}
> diff --git a/lib/librte_telemetry/telemetry_data.h b/lib/librte_telemetry/telemetry_data.h
> index ff3a371a3..adb84a09f 100644
> --- a/lib/librte_telemetry/telemetry_data.h
> +++ b/lib/librte_telemetry/telemetry_data.h
> @@ -15,6 +15,12 @@ enum tel_container_types {
> RTE_TEL_ARRAY_STRING, /** array of string values only */
> RTE_TEL_ARRAY_INT, /** array of signed, 32-bit int values */
> RTE_TEL_ARRAY_U64, /** array of unsigned 64-bit int values */
> + RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
> +};
> +
> +struct container {
> + struct rte_tel_data *data;
> + int keep;
> };
>
> /* each type here must have an equivalent enum in the value types enum in
> @@ -25,6 +31,7 @@ union tel_value {
> char sval[RTE_TEL_MAX_STRING_LEN];
> int ival;
> uint64_t u64val;
> + struct container container;
> };
>
> struct tel_dict_entry {
> diff --git a/lib/librte_telemetry/telemetry_json.h b/lib/librte_telemetry/telemetry_json.h
> index a2ce4899e..ad270b9b3 100644
> --- a/lib/librte_telemetry/telemetry_json.h
> +++ b/lib/librte_telemetry/telemetry_json.h
> @@ -102,6 +102,22 @@ rte_tel_json_add_array_u64(char *buf, const int len, const int used,
> return ret == 0 ? used : end + ret;
> }
>
> +/*
> + * Add a new element with raw JSON value to the JSON array stored in the
> + * provided buffer.
> + */
> +static inline int
> +rte_tel_json_add_array_json(char *buf, const int len, const int used,
> + const char *str)
> +{
> + int ret, end = used - 1; /* strip off final delimiter */
> + if (used <= 2) /* assume empty, since minimum is '[]' */
> + return __json_snprintf(buf, len, "[%s]", str);
> +
> + ret = __json_snprintf(buf + end, len - end, ",%s]", str);
> + return ret == 0 ? used : end + ret;
> +}
> +
> /**
> * Add a new element with uint64_t value to the JSON object stored in the
> * provided buffer.
> @@ -155,4 +171,21 @@ rte_tel_json_add_obj_str(char *buf, const int len, const int used,
> return ret == 0 ? used : end + ret;
> }
>
> +/**
> + * Add a new element with raw JSON value to the JSON object stored in the
> + * provided buffer.
> + */
> +static inline int
> +rte_tel_json_add_obj_json(char *buf, const int len, const int used,
> + const char *name, const char *val)
> +{
> + int ret, end = used - 1;
> + if (used <= 2) /* assume empty, since minimum is '{}' */
> + return __json_snprintf(buf, len, "{\"%s\":%s}", name, val);
> +
> + ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> + name, val);
> + return ret == 0 ? used : end + ret;
> +}
> +
> #endif /*_RTE_TELEMETRY_JSON_H_*/
> --
> 2.17.1
>