[v1,1/3] telemetry: enable storing pointer value

Message ID f4ddcb45d45e7b5b700cacebd02f1946b52ea368.1627572033.git.gmuthukrishn@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series common/cnxk: enable npa telemetry |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Gowrishankar Muthukrishnan July 29, 2021, 3:25 p.m. UTC
  At present, value of pointer variable or address can only be
stored in u64 type which is slightly not human readable, hence
this patch is. It adds telemetry support to store pointer value,
which is stringified.

Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
---
 lib/telemetry/rte_telemetry.h  | 37 ++++++++++++++++++++++++++++++-
 lib/telemetry/telemetry.c      | 21 ++++++++++++++++--
 lib/telemetry/telemetry_data.c | 40 ++++++++++++++++++++++++++++++----
 lib/telemetry/telemetry_data.h |  2 ++
 lib/telemetry/telemetry_json.h | 31 ++++++++++++++++++++++++++
 lib/telemetry/version.map      |  2 ++
 6 files changed, 126 insertions(+), 7 deletions(-)
  

Comments

Bruce Richardson July 29, 2021, 3:48 p.m. UTC | #1
On Thu, Jul 29, 2021 at 08:55:35PM +0530, Gowrishankar Muthukrishnan wrote:
> At present, value of pointer variable or address can only be
> stored in u64 type which is slightly not human readable, hence
> this patch is. It adds telemetry support to store pointer value,
> which is stringified.
> 
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> ---

I'm a little curious as to the usefulness of having a pointer value in
telemetry output? How would a telemetry user be expected to use pointer
information returned? Printing pointers seems something more useful for a
debugging or tracing interface than a telemetry one.

Regards,
/Bruce
  
Gowrishankar Muthukrishnan July 30, 2021, 12:08 p.m. UTC | #2
Hi Bruce,

> I'm a little curious as to the usefulness of having a pointer value in telemetry
> output? How would a telemetry user be expected to use pointer information
> returned? Printing pointers seems something more useful for a debugging or
> tracing interface than a telemetry one.
> 

Thanks for the quick review. I enabled _ptr API keeping few things in mind:

1. User need to explicitly type cast pointer value (ie address) to uint64_t
    which otherwise can cause compiler warning (Wint-conversion). Although
    u64 is large enough for holding address as value, type casting is problematic
    for non-64 bit machines (eg 32 bit). One other option is to use uintptr_t
    as a holder.

2. With this API, code walk could be easier as user can interpret the accessed
     data better (ie ptr is address value). _ptr API is meant for pointer variables,
     though it is up to user to choose.

3. Also while debugging telemetry date using script like usertools/dpdk-telemetry.py,
    perceiving address as hex is quicker than same as u64.

Answering on returned data, user needs to convert stringified hex to pointer value.

Regards,
Gowrishankar
  
Gowrishankar Muthukrishnan Aug. 1, 2021, 5:40 p.m. UTC | #3
> -----Original Message-----
> From: Gowrishankar Muthukrishnan
> Sent: Friday, July 30, 2021 5:38 PM
> To: Bruce Richardson <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; ciara.power@intel.com; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Kiran Kumar Kokkilagadda <kirankumark@marvell.com>;
> Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Sunil Kumar Kori
> <skori@marvell.com>; Satha Koteswara Rao Kottidi
> <skoteshwar@marvell.com>
> Subject: RE: [EXT] Re: [dpdk-dev] [v1, 1/3] telemetry: enable storing pointer
> value
> 
> Hi Bruce,
> 
> > I'm a little curious as to the usefulness of having a pointer value in
> > telemetry output? How would a telemetry user be expected to use
> > pointer information returned? Printing pointers seems something more
> > useful for a debugging or tracing interface than a telemetry one.
> >
> 
> Thanks for the quick review. I enabled _ptr API keeping few things in mind:
> 
> 1. User need to explicitly type cast pointer value (ie address) to uint64_t
>     which otherwise can cause compiler warning (Wint-conversion). Although
>     u64 is large enough for holding address as value, type casting is problematic
>     for non-64 bit machines (eg 32 bit). One other option is to use uintptr_t
>     as a holder.
> 
Please check [v2].
I modified json encoding to uintptr instead of stringified hex as in current patch.
I think, this is better approach as pointer value is stored more correctly (void *)
as well as retrieved in JSON following uintptr_t cast. Also I think, this is architecture 
compliant approach rather than assuming pointer address is always 64 bit (and 
what if 128 bit comes alive - who knows when). Aim is to leave _ptr api 
architecture compliance anytime.

> 2. With this API, code walk could be easier as user can interpret the accessed
>      data better (ie ptr is address value). _ptr API is meant for pointer variables,
>      though it is up to user to choose.
> 
With above uintptr_t as encoded value, it does not change the client handling
as client would consume it as its architecture supported value (uint64_t).
One advantage of having this API is to support JSON5 compliance hex address
once decided so in future.

> 3. Also while debugging telemetry date using script like usertools/dpdk-
> telemetry.py,
>     perceiving address as hex is quicker than same as u64.
> 
> Answering on returned data, user needs to convert stringified hex to pointer
> value.
With uintptr_t value (in new patch), no change is needed in client side.

Please suggest.
Thanks,
Gowrishankar
> 
> Regards,
> Gowrishankar
  

Patch

diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index 8776998b54..4c453d35d7 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -46,7 +46,8 @@  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 */
+	RTE_TEL_CONTAINER,  /** a container struct */
+	RTE_TEL_PTR_VAL,    /** a pointer value */
 };
 
 /**
@@ -137,6 +138,22 @@  __rte_experimental
 int
 rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x);
 
+/**
+ * Add a pointer value to an array.
+ * The array must have been started by rte_tel_data_start_array() with
+ * RTE_TEL_PTR_VAL as the type parameter.
+ *
+ * @param d
+ *   The data structure passed to the callback
+ * @param x
+ *   The pointer value to be returned in the array
+ * @return
+ *   0 on success, negative errno on error
+ */
+__rte_experimental
+int
+rte_tel_data_add_array_ptr(struct rte_tel_data *d, void *x);
+
 /**
  * Add a container to an array. A container is an existing telemetry data
  * array. The array the container is to be added to must have been started by
@@ -213,6 +230,24 @@  int
 rte_tel_data_add_dict_u64(struct rte_tel_data *d,
 		const char *name, uint64_t val);
 
+/**
+ * Add a pointer value 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 name
+ *   The name the value is to be stored under in the dict
+ * @param val
+ *   The pointer value to be stored in the dict
+ * @return
+ *   0 on success, negative errno on error, E2BIG on string truncation of name.
+ */
+__rte_experimental
+int
+rte_tel_data_add_dict_ptr(struct rte_tel_data *d,
+		const char *name, void *ptr);
+
 /**
  * Add a container to a dictionary. A container is an existing telemetry data
  * array. The dict the container is to be added to must have been started by
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8665db8d03..5842b28740 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -157,8 +157,10 @@  container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 	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)
+	if (d->type != RTE_TEL_ARRAY_U64
+		&& d->type != RTE_TEL_ARRAY_INT
+		&& d->type != RTE_TEL_ARRAY_PTR
+		&& d->type != RTE_TEL_ARRAY_STRING)
 		return snprintf(out_buf, buf_len, "null");
 
 	used = rte_tel_json_empty_array(out_buf, buf_len, 0);
@@ -167,6 +169,11 @@  container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 			used = rte_tel_json_add_array_u64(out_buf,
 				buf_len, used,
 				d->data.array[i].u64val);
+	if (d->type == RTE_TEL_ARRAY_PTR)
+		for (i = 0; i < d->data_len; i++)
+			used = rte_tel_json_add_array_ptr(out_buf,
+				buf_len, used,
+				d->data.array[i].ptrval);
 	if (d->type == RTE_TEL_ARRAY_INT)
 		for (i = 0; i < d->data_len; i++)
 			used = rte_tel_json_add_array_int(out_buf,
@@ -226,6 +233,11 @@  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_PTR_VAL:
+				used = rte_tel_json_add_obj_ptr(cb_data_buf,
+						buf_len, used,
+						v->name, v->value.ptrval);
+				break;
 			case RTE_TEL_CONTAINER:
 			{
 				char temp[buf_len];
@@ -248,6 +260,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_PTR:
 	case RTE_TEL_ARRAY_CONTAINER:
 		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
 				MAX_CMD_LEN, cmd);
@@ -269,6 +282,10 @@  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_PTR)
+				used = rte_tel_json_add_array_ptr(cb_data_buf,
+						buf_len, used,
+						d->data.array[i].ptrval);
 			else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
 				char temp[buf_len];
 				const struct container *rec_data =
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 77b0fe09a5..9384f48589 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -15,6 +15,7 @@  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
 			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 */
+			RTE_TEL_ARRAY_PTR,    /* RTE_TEL_PTR_VAL = 4 */
 	};
 	d->type = array_types[type];
 	d->data_len = 0;
@@ -75,6 +76,17 @@  rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
 	return 0;
 }
 
+int
+rte_tel_data_add_array_ptr(struct rte_tel_data *d, void *x)
+{
+	if (d->type != RTE_TEL_ARRAY_PTR)
+		return -EINVAL;
+	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
+		return -ENOSPC;
+	d->data.array[d->data_len++].ptrval = x;
+	return 0;
+}
+
 int
 rte_tel_data_add_array_container(struct rte_tel_data *d,
 		struct rte_tel_data *val, int keep)
@@ -82,7 +94,8 @@  rte_tel_data_add_array_container(struct rte_tel_data *d,
 	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))
+			&& val->type != RTE_TEL_ARRAY_STRING
+			&& val->type != RTE_TEL_ARRAY_PTR))
 		return -EINVAL;
 	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
 		return -ENOSPC;
@@ -147,15 +160,34 @@  rte_tel_data_add_dict_u64(struct rte_tel_data *d,
 	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
 }
 
+int
+rte_tel_data_add_dict_ptr(struct rte_tel_data *d,
+		const char *name, void *ptr)
+{
+	struct tel_dict_entry *e = &d->data.dict[d->data_len];
+	if (d->type != RTE_TEL_DICT)
+		return -EINVAL;
+	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
+		return -ENOSPC;
+
+	d->data_len++;
+	e->type = RTE_TEL_PTR_VAL;
+	e->value.ptrval = ptr;
+	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))
+	if (d->type != RTE_TEL_DICT ||
+		(val->type != RTE_TEL_ARRAY_U64
+		 && val->type != RTE_TEL_ARRAY_INT
+		 && val->type != RTE_TEL_ARRAY_STRING
+		 && val->type != RTE_TEL_ARRAY_PTR))
 		return -EINVAL;
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
index adb84a09f1..bb361e3bcc 100644
--- a/lib/telemetry/telemetry_data.h
+++ b/lib/telemetry/telemetry_data.h
@@ -16,6 +16,7 @@  enum tel_container_types {
 	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 */
+	RTE_TEL_ARRAY_PTR,    /** array of pointer values */
 };
 
 struct container {
@@ -31,6 +32,7 @@  union tel_value {
 	char sval[RTE_TEL_MAX_STRING_LEN];
 	int ival;
 	uint64_t u64val;
+	void *ptrval;
 	struct container container;
 };
 
diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index ad270b9b30..96762fc267 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -102,6 +102,19 @@  rte_tel_json_add_array_u64(char *buf, const int len, const int used,
 	return ret == 0 ? used : end + ret;
 }
 
+/* Appends a pointer value into the JSON array in the provided buffer. */
+static inline int
+rte_tel_json_add_array_ptr(char *buf, const int len, const int used,
+		void *ptr)
+{
+	int ret, end = used - 1; /* strip off final delimiter */
+	if (used <= 2) /* assume empty, since minimum is '[]' */
+		return __json_snprintf(buf, len, "[\"%p\"]", ptr);
+
+	ret = __json_snprintf(buf + end, len - end, ",\"%p\"]", ptr);
+	return ret == 0 ? used : end + ret;
+}
+
 /*
  * Add a new element with raw JSON value to the JSON array stored in the
  * provided buffer.
@@ -136,6 +149,24 @@  rte_tel_json_add_obj_u64(char *buf, const int len, const int used,
 	return ret == 0 ? used : end + ret;
 }
 
+/**
+ * Add a new element with uint64_t value to the JSON object stored in the
+ * provided buffer.
+ */
+static inline int
+rte_tel_json_add_obj_ptr(char *buf, const int len, const int used,
+		const char *name, void *ptr)
+{
+	int ret, end = used - 1;
+	if (used <= 2) /* assume empty, since minimum is '{}' */
+		return __json_snprintf(buf, len, "{\"%s\":\"%p\"}", name,
+				ptr);
+
+	ret = __json_snprintf(buf + end, len - end, ",\"%s\":\"%p\"}",
+			name, ptr);
+	return ret == 0 ? used : end + ret;
+}
+
 /**
  * Add a new element with int value to the JSON object stored in the
  * provided buffer.
diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
index bde80ce29b..d919340bc6 100644
--- a/lib/telemetry/version.map
+++ b/lib/telemetry/version.map
@@ -5,10 +5,12 @@  EXPERIMENTAL {
 	rte_tel_data_add_array_int;
 	rte_tel_data_add_array_string;
 	rte_tel_data_add_array_u64;
+	rte_tel_data_add_array_ptr;
 	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_add_dict_ptr;
 	rte_tel_data_alloc;
 	rte_tel_data_free;
 	rte_tel_data_start_array;