From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 To: Ciara Power 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > A number of comments inline below. Once fixed you can include my ack on V3. Acked-by: Bruce Richardson > --- > 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 >