telemetry logging improvements and cleanup

PATCH Superseded Bruce Richardson 4 participants 19 messages Mar 10, 2021 – Mar 25, 2021

Share & tools

Bruce Richardson

author

2021-03-10 17:24 UTC #0 lore raw

cover telemetry logging improvements and cleanup

This set adds support for using the regular rte_log functions from the telemetry library; avoiding circular dependencies by having EAL register the telemetry library itself and then passing the required handles to that library as part of the telemetry_init call.

Beyond this change, the other three patches are cleanups to ensure that all internal functions are clearly separate from the public APIs. (Patches 3 & 4 may be merged into a single one on apply, for I've kept them separate for now for clarity).

Bruce Richardson (4):
telemetry: use rte_log for logging
telemetry: make the legacy registration function internal
telemetry: create internal-only header file
telemetry: move init function to internal header

 doc/guides/rel_notes/release_21_05.rst        |  5 ++
 lib/librte_eal/freebsd/eal.c                  | 12 +--
 lib/librte_eal/linux/eal.c                    | 12 +--
 lib/librte_metrics/rte_metrics_telemetry.c    |  2 +-
 lib/librte_telemetry/rte_telemetry.h          | 22 ------
 lib/librte_telemetry/telemetry.c              | 74 +++++++++----------
 ...elemetry_legacy.h => telemetry_internal.h} | 37 +++++++++-
 lib/librte_telemetry/telemetry_legacy.c       |  2 +-
 lib/librte_telemetry/version.map              |  2 +-
 9 files changed, 82 insertions(+), 86 deletions(-)
 rename lib/librte_telemetry/{rte_telemetry_legacy.h => telemetry_internal.h} (67%)

--
2.27.0

Bruce Richardson

author

2021-03-10 17:24 UTC #1 lore raw

PATCH 1/4 telemetry: use rte_log for logging

Rather than passing back an error string to the caller, take as input the rte_log function to use, and just use regular logging.

Signed-off-by Bruce Richardson <[email protected]>

---
 lib/librte_eal/freebsd/eal.c         | 10 ++--
 lib/librte_eal/linux/eal.c           | 10 ++--
 lib/librte_telemetry/rte_telemetry.h | 15 ++++--
 lib/librte_telemetry/telemetry.c     | 72 +++++++++++++---------------
 4 files changed, 49 insertions(+), 58 deletions(-)

lib/librte_eal/freebsd/eal.c +3 -7

index e2cdad5283..b8a798bc7d 100644
@@ -941,15 +941,11 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 	if (!internal_conf->no_telemetry) {
-		const char *error_str = NULL;
+		uint32_t tlog = rte_log_register_type_and_pick_level(
+				"lib.telemetry", RTE_LOG_WARNING);
 		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
-				&internal_conf->ctrl_cpuset, &error_str)
-				!= 0) {
-			rte_eal_init_alert(error_str);
+				&internal_conf->ctrl_cpuset, rte_log, tlog) != 0)
 			return -1;
-		}
-		if (error_str != NULL)
-			RTE_LOG(NOTICE, EAL, "%s\n", error_str);
 	}
 
 	eal_mcfg_complete();

lib/librte_eal/linux/eal.c +3 -7

index 6c34ac8903..23e3a32036 100644
@@ -1314,15 +1314,11 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 	if (!internal_conf->no_telemetry) {
-		const char *error_str = NULL;
+		uint32_t tlog = rte_log_register_type_and_pick_level(
+				"lib.telemetry", RTE_LOG_WARNING);
 		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
-				&internal_conf->ctrl_cpuset, &error_str)
-				!= 0) {
-			rte_eal_init_alert(error_str);
+				&internal_conf->ctrl_cpuset, rte_log, tlog) != 0)
 			return -1;
-		}
-		if (error_str != NULL)
-			RTE_LOG(NOTICE, EAL, "%s\n", error_str);
 	}
 
 	eal_mcfg_complete();

lib/librte_telemetry/rte_telemetry.h +11 -4

index f8e54dc68e..2c3da3ab7f 100644
@@ -292,6 +292,12 @@ __rte_experimental
 int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
 
+/**
+ * @internal
+ * Log function type, to allow passing as parameter if necessary
+ */
+typedef int (*rte_log_fn)(uint32_t level, uint32_t logtype, const char *format, ...);
+
 /**
  * @internal
  * Initialize Telemetry.

@@ -300,9 +306,10 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
  * The runtime directory of DPDK.
  * @param cpuset
  * The CPU set to be used for setting the thread affinity.
- * @param err_str
- * This err_str pointer should point to NULL on entry. In the case of an error
- * or warning, it will be non-NULL on exit.
+ * @param log_fn
+ * Function pointer to the rte_log function for logging use
+ * @param registered_logtype
+ * The registered log type to use for logging
  *
  * @return
  *  0 on success.

@@ -312,7 +319,7 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
 __rte_internal
 int
 rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
-		const char **err_str);
+		rte_log_fn log_fn, uint32_t registered_logtype);
 
 /**
  * Get a pointer to a container with memory allocated. The container is to be

lib/librte_telemetry/telemetry.c +32 ~~-40~~

155 lines not on this page · raw message

--
2.27.0

Power, Ciara

Acked-by on v1 1/4

2021-03-11 12:50 UTC #2 lore raw

Re: [dpdk-dev] [PATCH 1/4] telemetry: use rte_log for logging

Quoted 20 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of Bruce Richardson
Sent: Wednesday 10 March 2021 17:24
To: [email protected]
Cc: Richardson, Bruce <[email protected]>; Laatz, Kevin <[email protected]>
Subject: [dpdk-dev] [PATCH 1/4] telemetry: use rte_log for logging
Rather than passing back an error string to the caller, take as input the rte_log function to use, and just use regular logging.
Signed-off-by Bruce Richardson <[email protected]>
---
lib/librte_eal/freebsd/eal.c         | 10 ++--
lib/librte_eal/linux/eal.c           | 10 ++--
lib/librte_telemetry/rte_telemetry.h | 15 ++++--
lib/librte_telemetry/telemetry.c     | 72 +++++++++++++---------------
4 files changed, 49 insertions(+), 58 deletions(-)

<snip>

Acked-by Ciara Power <[email protected]>

Bruce Richardson

author

2021-03-10 17:24 UTC #3 lore raw

PATCH 2/4 telemetry: make the legacy registration function internal

In reply to Bruce Richardson

The function for registration of callbacks for legacy telemetry was documented as internal-only in the API documents, but marked as experimental in the version.map file. Since this is an internal-only function, for consistency we update the version mapping to have it as internal.

Signed-off-by Bruce Richardson <[email protected]>

---
 doc/guides/rel_notes/release_21_05.rst      | 5 +++++
 lib/librte_telemetry/rte_telemetry_legacy.h | 2 +-
 lib/librte_telemetry/version.map            | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

doc/guides/rel_notes/release_21_05.rst +5 -0

index 23f7f0bff9..1624f43d73 100644
@@ -117,6 +117,11 @@ ABI Changes
 
 * No ABI change that would break compatibility with 20.11.
 
+* The experimental function ``rte_telemetry_legacy_register`` has been
+  removed from the public API and is now an internal-only function. This
+  function was already marked as internal in the API documentation for it,
+  and was not for use by external applications.
+
 
 Known Issues
 ------------

lib/librte_telemetry/rte_telemetry_legacy.h +1 -1

index c83f9a8d90..fb44740186 100644
@@ -78,7 +78,7 @@ legacy_client_handler(void *sock_id);
  *  @return
  *  -ENOENT if max callbacks limit has been reached.
  */
-__rte_experimental
+__rte_internal
 int
 rte_telemetry_legacy_register(const char *cmd,
 		enum rte_telemetry_legacy_data_req data_req,

lib/librte_telemetry/version.map +1 -1

index ec0ebc1bec..bde80ce29b 100644
@@ -14,12 +14,12 @@ EXPERIMENTAL {
 	rte_tel_data_start_array;
 	rte_tel_data_start_dict;
 	rte_tel_data_string;
-	rte_telemetry_legacy_register;
 	rte_telemetry_register_cmd;
 
 	local: *;
 };
 
 INTERNAL {
+	rte_telemetry_legacy_register;
 	rte_telemetry_init;
 };

Signature · 1 line

2.27.0

Power, Ciara

Acked-by on v1 2/4

2021-03-11 12:50 UTC #4 lore raw

Re: [dpdk-dev] [PATCH 2/4] telemetry: make the legacy registration function internal

Quoted 23 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of Bruce Richardson
Sent: Wednesday 10 March 2021 17:24
To: [email protected]
Cc: Richardson, Bruce <[email protected]>; Laatz, Kevin <[email protected]>; Ray Kinsella <[email protected]>; Neil Horman <[email protected]>
Subject: [dpdk-dev] [PATCH 2/4] telemetry: make the legacy registration function internal
The function for registration of callbacks for legacy telemetry was documented as internal-only in the API documents, but marked as experimental in the version.map file. Since this is an internal-only function, for consistency we update the version mapping to have it as internal.
Signed-off-by Bruce Richardson <[email protected]>
---
doc/guides/rel_notes/release_21_05.rst      | 5 +++++
lib/librte_telemetry/rte_telemetry_legacy.h | 2 +-
lib/librte_telemetry/version.map            | 2 +-
3 files changed, 7 insertions(+), 2 deletions(-)

<snip>

Acked-by Ciara Power <[email protected]>

Bruce Richardson

author

2021-03-10 17:25 UTC #5 lore raw

PATCH 3/4 telemetry: create internal-only header file

In reply to Bruce Richardson

The header file containing the legacy telemetry function prototypes was all internal-only, so we rename the file to be an internal-only one to make it clearer it's not for installation.

Signed-off-by Bruce Richardson <[email protected]>

---
 lib/librte_metrics/rte_metrics_telemetry.c                  | 2 +-
 lib/librte_telemetry/telemetry.c                            | 2 +-
 .../{rte_telemetry_legacy.h => telemetry_internal.h}        | 6 +++---
 lib/librte_telemetry/telemetry_legacy.c                     | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)
 rename lib/librte_telemetry/{rte_telemetry_legacy.h => telemetry_internal.h} (93%)

lib/librte_metrics/rte_metrics_telemetry.c +1 -1

index b8ee56ef01..bbc6a79f88 100644
@@ -7,7 +7,7 @@
 #include <rte_ethdev.h>
 #include <rte_string_fns.h>
 #ifdef RTE_LIB_TELEMETRY
-#include <rte_telemetry_legacy.h>
+#include <telemetry_internal.h>
 #endif
 
 #include "rte_metrics.h"

lib/librte_telemetry/telemetry.c +1 -1

index 18f2ae2e2f..c3c988e972 100644
@@ -21,7 +21,7 @@
 #include "rte_telemetry.h"
 #include "telemetry_json.h"
 #include "telemetry_data.h"
-#include "rte_telemetry_legacy.h"
+#include "telemetry_internal.h"
 
 #define MAX_CMD_LEN 56
 #define MAX_HELP_LEN 64

lib/librte_telemetry/telemetry_internal.h renamed from lib/librte_telemetry/rte_telemetry_legacy.h +3 -3

similarity index 93%
rename from lib/librte_telemetry/rte_telemetry_legacy.h
rename to lib/librte_telemetry/telemetry_internal.h
index fb44740186..ad076b9119 100644
@@ -2,8 +2,8 @@
  * Copyright(c) 2020 Intel Corporation
  */
 
-#ifndef _RTE_TELEMETRY_LEGACY_H_
-#define _RTE_TELEMETRY_LEGACY_H_
+#ifndef _RTE_TELEMETRY_INTERNAL_H_
+#define _RTE_TELEMETRY_INTERNAL_H_
 
 #include <rte_compat.h>
 #include "rte_telemetry.h"

@@ -14,7 +14,7 @@
  * @b EXPERIMENTAL: this API may change without prior notice
 
  * @file
- * RTE Telemetry Legacy
+ * RTE Telemetry Legacy and internal definitions
  *
  ***/

lib/librte_telemetry/telemetry_legacy.c +1 -1

index edd76ca359..5e9af37db1 100644
@@ -15,7 +15,7 @@
 #include <rte_common.h>
 #include <rte_spinlock.h>
 
-#include "rte_telemetry_legacy.h"
+#include "telemetry_internal.h"
 
 #define MAX_LEN 128
 #define BUF_SIZE 1024

Signature · 1 line

2.27.0

Power, Ciara

Acked-by on v1 3/4

2021-03-11 12:51 UTC #6 lore raw

Re: [dpdk-dev] [PATCH 3/4] telemetry: create internal-only header file

Quoted 22 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of Bruce Richardson
Sent: Wednesday 10 March 2021 17:24
To: [email protected]
Cc: Richardson, Bruce <[email protected]>; Laatz, Kevin <[email protected]>
Subject: [dpdk-dev] [PATCH 3/4] telemetry: create internal-only header file
The header file containing the legacy telemetry function prototypes was all internal-only, so we rename the file to be an internal-only one to make it clearer it's not for installation.
Signed-off-by Bruce Richardson <[email protected]>
---
lib/librte_metrics/rte_metrics_telemetry.c                  | 2 +-
lib/librte_telemetry/telemetry.c                            | 2 +-
.../{rte_telemetry_legacy.h => telemetry_internal.h}        | 6 +++---
lib/librte_telemetry/telemetry_legacy.c                     | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)  rename
lib/librte_telemetry/{rte_telemetry_legacy.h => telemetry_internal.h} (93%)

<snip>

Acked-by Ciara Power <[email protected]>

Bruce Richardson

author

2021-03-10 17:25 UTC #7 lore raw

PATCH 4/4 telemetry: move init function to internal header

In reply to Bruce Richardson

The rte_telemetry_init() function is for EAL use only, so can be moved to the internal header rather than being in the public one.

Signed-off-by Bruce Richardson <[email protected]>

---
 lib/librte_eal/freebsd/eal.c              |  2 +-
 lib/librte_eal/linux/eal.c                |  2 +-
 lib/librte_telemetry/rte_telemetry.h      | 29 -----------------------
 lib/librte_telemetry/telemetry_internal.h | 29 +++++++++++++++++++++++
 4 files changed, 31 insertions(+), 31 deletions(-)

lib/librte_eal/freebsd/eal.c +1 -1

index b8a798bc7d..be02f70510 100644
@@ -42,7 +42,7 @@
 #include <rte_version.h>
 #include <rte_vfio.h>
 #include <malloc_heap.h>
-#include <rte_telemetry.h>
+#include <telemetry_internal.h>
 
 #include "eal_private.h"
 #include "eal_thread.h"

lib/librte_eal/linux/eal.c +1 -1

index 23e3a32036..da2d6602a8 100644
@@ -49,8 +49,8 @@
 #include <rte_version.h>
 #include <malloc_heap.h>
 #include <rte_vfio.h>
-#include <rte_telemetry.h>
 
+#include <telemetry_internal.h>
 #include "eal_private.h"
 #include "eal_thread.h"
 #include "eal_internal_cfg.h"

lib/librte_telemetry/rte_telemetry.h +0 ~~-29~~

index 2c3da3ab7f..334ea8ddd4 100644
@@ -292,35 +292,6 @@ __rte_experimental
 int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
 
-/**
- * @internal
- * Log function type, to allow passing as parameter if necessary
- */
-typedef int (*rte_log_fn)(uint32_t level, uint32_t logtype, const char *format, ...);
-
-/**
- * @internal
- * Initialize Telemetry.
- *
- * @param runtime_dir
- * The runtime directory of DPDK.
- * @param cpuset
- * The CPU set to be used for setting the thread affinity.
- * @param log_fn
- * Function pointer to the rte_log function for logging use
- * @param registered_logtype
- * The registered log type to use for logging
- *
- * @return
- *  0 on success.
- * @return
- *  -1 on failure.
- */
-__rte_internal
-int
-rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
-		rte_log_fn log_fn, uint32_t registered_logtype);
-
 /**
  * Get a pointer to a container with memory allocated. The container is to be
  * used embedded within an existing telemetry dict/array.

lib/librte_telemetry/telemetry_internal.h +29 -0

index ad076b9119..601f16cc96 100644
@@ -84,4 +84,33 @@ rte_telemetry_legacy_register(const char *cmd,
 		enum rte_telemetry_legacy_data_req data_req,
 		telemetry_legacy_cb fn);
 
+/**
+ * @internal
+ * Log function type, to allow passing as parameter if necessary
+ */
+typedef int (*rte_log_fn)(uint32_t level, uint32_t logtype, const char *format, ...);
+
+/**
+ * @internal
+ * Initialize Telemetry.
+ *
+ * @param runtime_dir
+ * The runtime directory of DPDK.
+ * @param cpuset
+ * The CPU set to be used for setting the thread affinity.
+ * @param log_fn
+ * Function pointer to the rte_log function for logging use
+ * @param registered_logtype
+ * The registered log type to use for logging
+ *
+ * @return
+ *  0 on success.
+ * @return
+ *  -1 on failure.
+ */
+__rte_internal
+int
+rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
+		rte_log_fn log_fn, uint32_t registered_logtype);
+
 #endif

Signature · 1 line

2.27.0

Power, Ciara

Acked-by on v1 4/4

2021-03-11 12:51 UTC #8 lore raw

Re: [dpdk-dev] [PATCH 4/4] telemetry: move init function to internal header

Quoted 21 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of Bruce Richardson
Sent: Wednesday 10 March 2021 17:25
To: [email protected]
Cc: Richardson, Bruce <[email protected]>; Laatz, Kevin <[email protected]>
Subject: [dpdk-dev] [PATCH 4/4] telemetry: move init function to internal header
The rte_telemetry_init() function is for EAL use only, so can be moved to the internal header rather than being in the public one.
Signed-off-by Bruce Richardson <[email protected]>
---
lib/librte_eal/freebsd/eal.c              |  2 +-
lib/librte_eal/linux/eal.c                |  2 +-
lib/librte_telemetry/rte_telemetry.h      | 29 -----------------------
lib/librte_telemetry/telemetry_internal.h | 29 +++++++++++++++++++++++
4 files changed, 31 insertions(+), 31 deletions(-)

<snip>

Acked-by Ciara Power <[email protected]>

Thomas Monjalon

2021-03-24 21:12 UTC #9 lore raw

In reply to Bruce Richardson

10/03/2021 18:24, Bruce Richardson:

Quoted 10 lines

This set adds support for using the regular rte_log functions from the telemetry library; avoiding circular dependencies by having EAL register the telemetry library itself and then passing the required handles to that library as part of the telemetry_init call.
Beyond this change, the other three patches are cleanups to ensure that all internal functions are clearly separate from the public APIs. (Patches 3 & 4 may be merged into a single one on apply, for I've kept them separate for now for clarity).

Quoted 5 lines

Bruce Richardson (4):
telemetry: use rte_log for logging
telemetry: make the legacy registration function internal
telemetry: create internal-only header file
telemetry: move init function to internal header

Now that your patch "eal: fix querying DPDK version at runtime" is in main branch, please could you rebase this series?

Bruce Richardson

author

2021-03-25 08:55 UTC #10 lore raw

Quoted 11 lines · Thomas Monjalon

10/03/2021 18:24, Bruce Richardson:
Quoted 10 lines
This set adds support for using the regular rte_log functions from the telemetry library; avoiding circular dependencies by having EAL register the telemetry library itself and then passing the required handles to that library as part of the telemetry_init call.
Beyond this change, the other three patches are cleanups to ensure that all internal functions are clearly separate from the public APIs. (Patches 3 & 4 may be merged into a single one on apply, for I've kept them separate for now for clarity).

Quoted 9 lines

Quoted 5 lines
Bruce Richardson (4):
telemetry: use rte_log for logging
telemetry: make the legacy registration function internal
telemetry: create internal-only header file
telemetry: move init function to internal header
Now that your patch "eal: fix querying DPDK version at runtime" is in main branch, please could you rebase this series?

Sure, will do.

Bruce Richardson

author

2021-03-25 13:57 UTC #11 lore raw

v2 cover telemetry logging improvements and cleanup

In reply to Bruce Richardson

V2: Rebased on latest main branch.

Bruce Richardson (4):
telemetry: use rte_log for logging
telemetry: make the legacy registration function internal
telemetry: rename internal-only header file
telemetry: move init function to internal header

 doc/guides/rel_notes/release_21_05.rst        |  5 ++
 lib/librte_eal/freebsd/eal.c                  | 12 +--
 lib/librte_eal/linux/eal.c                    | 12 +--
 lib/librte_metrics/rte_metrics_telemetry.c    |  2 +-
 lib/librte_telemetry/rte_telemetry.h          | 25 ------
 lib/librte_telemetry/telemetry.c              | 76 +++++++++----------
 ...elemetry_legacy.h => telemetry_internal.h} | 41 +++++++++-
 lib/librte_telemetry/telemetry_legacy.c       |  2 +-
 lib/librte_telemetry/version.map              |  2 +-
 9 files changed, 87 insertions(+), 90 deletions(-)
 rename lib/librte_telemetry/{rte_telemetry_legacy.h => telemetry_internal.h} (65%)

--
2.27.0

Bruce Richardson

author

2021-03-25 13:57 UTC #12 lore raw

PATCH v2 1/4 telemetry: use rte_log for logging

Rather than passing back an error string to the caller, take as input the rte_log function to use, and just use regular logging.

Signed-off-by Bruce Richardson <[email protected]>

Acked-by Ciara Power <[email protected]>

---
 lib/librte_eal/freebsd/eal.c         | 10 ++--
 lib/librte_eal/linux/eal.c           | 10 ++--
 lib/librte_telemetry/rte_telemetry.h | 15 ++++--
 lib/librte_telemetry/telemetry.c     | 74 +++++++++++++---------------
 4 files changed, 50 insertions(+), 59 deletions(-)

lib/librte_eal/freebsd/eal.c +3 -7

index 62320d610..97ce9976c 100644
@@ -941,16 +941,12 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 	if (!internal_conf->no_telemetry) {
-		const char *error_str = NULL;
+		uint32_t tlog = rte_log_register_type_and_pick_level(
+				"lib.telemetry", RTE_LOG_WARNING);
 		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
 				rte_version(),
-				&internal_conf->ctrl_cpuset, &error_str)
-				!= 0) {
-			rte_eal_init_alert(error_str);
+				&internal_conf->ctrl_cpuset, rte_log, tlog) != 0)
 			return -1;
-		}
-		if (error_str != NULL)
-			RTE_LOG(NOTICE, EAL, "%s\n", error_str);
 	}
 
 	eal_mcfg_complete();

lib/librte_eal/linux/eal.c +3 -7

index 9ffb4b331..f6dd67b6d 100644
@@ -1314,16 +1314,12 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 	if (!internal_conf->no_telemetry) {
-		const char *error_str = NULL;
+		uint32_t tlog = rte_log_register_type_and_pick_level(
+				"lib.telemetry", RTE_LOG_WARNING);
 		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
 				rte_version(),
-				&internal_conf->ctrl_cpuset, &error_str)
-				!= 0) {
-			rte_eal_init_alert(error_str);
+				&internal_conf->ctrl_cpuset, rte_log, tlog) != 0)
 			return -1;
-		}
-		if (error_str != NULL)
-			RTE_LOG(NOTICE, EAL, "%s\n", error_str);
 	}
 
 	eal_mcfg_complete();

lib/librte_telemetry/rte_telemetry.h +11 -4

index 027b048d7..d38894b97 100644
@@ -294,6 +294,12 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
 
 #ifdef RTE_HAS_CPUSET
 
+/**
+ * @internal
+ * Log function type, to allow passing as parameter if necessary
+ */
+typedef int (*rte_log_fn)(uint32_t level, uint32_t logtype, const char *format, ...);
+
 /**
  * @internal
  * Initialize Telemetry.

@@ -302,9 +308,10 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
  * The runtime directory of DPDK.
  * @param cpuset
  * The CPU set to be used for setting the thread affinity.
- * @param err_str
- * This err_str pointer should point to NULL on entry. In the case of an error
- * or warning, it will be non-NULL on exit.
+ * @param log_fn
+ * Function pointer to the rte_log function for logging use
+ * @param registered_logtype
+ * The registered log type to use for logging
  *
  * @return
  *  0 on success.

@@ -314,7 +321,7 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
 __rte_internal
 int
 rte_telemetry_init(const char *runtime_dir, const char *rte_version, rte_cpuset_t *cpuset,
-		const char **err_str);
+		rte_log_fn log_fn, uint32_t registered_logtype);
 
 #endif /* RTE_HAS_CPUSET */

lib/librte_telemetry/telemetry.c +33 ~~-41~~

158 lines not on this page · raw message

Signature · 1 line

2.27.0

David Marchand

2021-03-25 14:09 UTC #13 lore raw

Re: [dpdk-dev] [PATCH v2 1/4] telemetry: use rte_log for logging

Quoted 6 lines · Bruce Richardson

Rather than passing back an error string to the caller, take as input the rte_log function to use, and just use regular logging.
Signed-off-by Bruce Richardson <[email protected]>
Acked-by Ciara Power <[email protected]>

I guess this replaces
http://patchwork.dpdk.org/project/dpdk/patch/[email protected]/
?

Signature · 1 line

David Marchand

Bruce Richardson

author

2021-03-25 14:17 UTC #14 lore raw

Re: [dpdk-dev] [PATCH v2 1/4] telemetry: use rte_log for logging

Quoted 6 lines · David Marchand

Quoted 4 lines · Bruce Richardson
Rather than passing back an error string to the caller, take as input the rte_log function to use, and just use regular logging.

Quoted 6 lines

Quoted 2 lines
Signed-off-by Bruce Richardson <[email protected]>
Acked-by Ciara Power <[email protected]>
I guess this replaces
http://patchwork.dpdk.org/project/dpdk/patch/[email protected]/
?

Yes, it would do.

Bruce Richardson

author

2021-03-25 13:57 UTC #15 lore raw

PATCH v2 2/4 telemetry: make the legacy registration function internal

In reply to Bruce Richardson

Signed-off-by Bruce Richardson <[email protected]>

Acked-by Ciara Power <[email protected]>

---
 doc/guides/rel_notes/release_21_05.rst      | 5 +++++
 lib/librte_telemetry/rte_telemetry_legacy.h | 2 +-
 lib/librte_telemetry/version.map            | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

doc/guides/rel_notes/release_21_05.rst +5 -0

index 8e686cc62..fb965fec3 100644
@@ -148,6 +148,11 @@ ABI Changes
 
 * No ABI change that would break compatibility with 20.11.
 
+* The experimental function ``rte_telemetry_legacy_register`` has been
+  removed from the public API and is now an internal-only function. This
+  function was already marked as internal in the API documentation for it,
+  and was not for use by external applications.
+
 
 Known Issues
 ------------

lib/librte_telemetry/rte_telemetry_legacy.h +1 -1

index c83f9a8d9..fb4474018 100644
@@ -78,7 +78,7 @@ legacy_client_handler(void *sock_id);
  *  @return
  *  -ENOENT if max callbacks limit has been reached.
  */
-__rte_experimental
+__rte_internal
 int
 rte_telemetry_legacy_register(const char *cmd,
 		enum rte_telemetry_legacy_data_req data_req,

lib/librte_telemetry/version.map +1 -1

index ec0ebc1be..bde80ce29 100644
@@ -14,12 +14,12 @@ EXPERIMENTAL {
 	rte_tel_data_start_array;
 	rte_tel_data_start_dict;
 	rte_tel_data_string;
-	rte_telemetry_legacy_register;
 	rte_telemetry_register_cmd;
 
 	local: *;
 };
 
 INTERNAL {
+	rte_telemetry_legacy_register;
 	rte_telemetry_init;
 };

Signature · 1 line

2.27.0

Bruce Richardson

author

2021-03-25 13:57 UTC #16 lore raw

PATCH v2 3/4 telemetry: rename internal-only header file

In reply to Bruce Richardson

The header file containing the legacy telemetry function prototypes was all internal-only, so we rename the file to be an internal-only one to make it clearer it's not for installation.

Signed-off-by Bruce Richardson <[email protected]>

Acked-by Ciara Power <[email protected]>

---
 lib/librte_metrics/rte_metrics_telemetry.c                  | 2 +-
 lib/librte_telemetry/telemetry.c                            | 2 +-
 .../{rte_telemetry_legacy.h => telemetry_internal.h}        | 6 +++---
 lib/librte_telemetry/telemetry_legacy.c                     | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)
 rename lib/librte_telemetry/{rte_telemetry_legacy.h => telemetry_internal.h} (93%)

lib/librte_metrics/rte_metrics_telemetry.c +1 -1

index 795bd29fe..c24990d92 100644
@@ -5,7 +5,7 @@
 #include <rte_ethdev.h>
 #include <rte_string_fns.h>
 #ifdef RTE_LIB_TELEMETRY
-#include <rte_telemetry_legacy.h>
+#include <telemetry_internal.h>
 #endif
 
 #include "rte_metrics.h"

lib/librte_telemetry/telemetry.c +1 -1

index 042136b82..7e08afd22 100644
@@ -20,7 +20,7 @@
 #include "rte_telemetry.h"
 #include "telemetry_json.h"
 #include "telemetry_data.h"
-#include "rte_telemetry_legacy.h"
+#include "telemetry_internal.h"
 
 #define MAX_CMD_LEN 56
 #define MAX_HELP_LEN 64

lib/librte_telemetry/telemetry_internal.h renamed from lib/librte_telemetry/rte_telemetry_legacy.h +3 -3

similarity index 93%
rename from lib/librte_telemetry/rte_telemetry_legacy.h
rename to lib/librte_telemetry/telemetry_internal.h
index fb4474018..ad076b911 100644
@@ -2,8 +2,8 @@
  * Copyright(c) 2020 Intel Corporation
  */
 
-#ifndef _RTE_TELEMETRY_LEGACY_H_
-#define _RTE_TELEMETRY_LEGACY_H_
+#ifndef _RTE_TELEMETRY_INTERNAL_H_
+#define _RTE_TELEMETRY_INTERNAL_H_
 
 #include <rte_compat.h>
 #include "rte_telemetry.h"

@@ -14,7 +14,7 @@
  * @b EXPERIMENTAL: this API may change without prior notice
 
  * @file
- * RTE Telemetry Legacy
+ * RTE Telemetry Legacy and internal definitions
  *
  ***/

lib/librte_telemetry/telemetry_legacy.c +1 -1

index edd76ca35..5e9af37db 100644
@@ -15,7 +15,7 @@
 #include <rte_common.h>
 #include <rte_spinlock.h>
 
-#include "rte_telemetry_legacy.h"
+#include "telemetry_internal.h"
 
 #define MAX_LEN 128
 #define BUF_SIZE 1024

Signature · 1 line

2.27.0

Bruce Richardson

author

2021-03-25 13:57 UTC #17 lore raw

PATCH v2 4/4 telemetry: move init function to internal header

In reply to Bruce Richardson

The rte_telemetry_init() function is for EAL use only, so can be moved to the internal header rather than being in the public one.

Signed-off-by Bruce Richardson <[email protected]>

Acked-by Ciara Power <[email protected]>

---
 lib/librte_eal/freebsd/eal.c              |  2 +-
 lib/librte_eal/linux/eal.c                |  2 +-
 lib/librte_telemetry/rte_telemetry.h      | 32 ----------------------
 lib/librte_telemetry/telemetry_internal.h | 33 +++++++++++++++++++++++
 4 files changed, 35 insertions(+), 34 deletions(-)

lib/librte_eal/freebsd/eal.c +1 -1

index 97ce9976c..32442e5ba 100644
@@ -42,7 +42,7 @@
 #include <rte_version.h>
 #include <rte_vfio.h>
 #include <malloc_heap.h>
-#include <rte_telemetry.h>
+#include <telemetry_internal.h>
 
 #include "eal_private.h"
 #include "eal_thread.h"

lib/librte_eal/linux/eal.c +1 -1

index f6dd67b6d..abbb53774 100644
@@ -49,8 +49,8 @@
 #include <rte_version.h>
 #include <malloc_heap.h>
 #include <rte_vfio.h>
-#include <rte_telemetry.h>
 
+#include <telemetry_internal.h>
 #include "eal_private.h"
 #include "eal_thread.h"
 #include "eal_internal_cfg.h"

lib/librte_telemetry/rte_telemetry.h +0 ~~-32~~

index d38894b97..fd57718c2 100644
@@ -292,38 +292,6 @@ __rte_experimental
 int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
 
-#ifdef RTE_HAS_CPUSET
-
-/**
- * @internal
- * Log function type, to allow passing as parameter if necessary
- */
-typedef int (*rte_log_fn)(uint32_t level, uint32_t logtype, const char *format, ...);
-
-/**
- * @internal
- * Initialize Telemetry.
- *
- * @param runtime_dir
- * The runtime directory of DPDK.
- * @param cpuset
- * The CPU set to be used for setting the thread affinity.
- * @param log_fn
- * Function pointer to the rte_log function for logging use
- * @param registered_logtype
- * The registered log type to use for logging
- *
- * @return
- *  0 on success.
- * @return
- *  -1 on failure.
- */
-__rte_internal
-int
-rte_telemetry_init(const char *runtime_dir, const char *rte_version, rte_cpuset_t *cpuset,
-		rte_log_fn log_fn, uint32_t registered_logtype);
-
-#endif /* RTE_HAS_CPUSET */
 
 /**
  * Get a pointer to a container with memory allocated. The container is to be

lib/librte_telemetry/telemetry_internal.h +33 -0

index ad076b911..6c5200604 100644
@@ -84,4 +84,37 @@ rte_telemetry_legacy_register(const char *cmd,
 		enum rte_telemetry_legacy_data_req data_req,
 		telemetry_legacy_cb fn);
 
+#ifdef RTE_HAS_CPUSET
+
+/**
+ * @internal
+ * Log function type, to allow passing as parameter if necessary
+ */
+typedef int (*rte_log_fn)(uint32_t level, uint32_t logtype, const char *format, ...);
+
+/**
+ * @internal
+ * Initialize Telemetry.
+ *
+ * @param runtime_dir
+ * The runtime directory of DPDK.
+ * @param cpuset
+ * The CPU set to be used for setting the thread affinity.
+ * @param log_fn
+ * Function pointer to the rte_log function for logging use
+ * @param registered_logtype
+ * The registered log type to use for logging
+ *
+ * @return
+ *  0 on success.
+ * @return
+ *  -1 on failure.
+ */
+__rte_internal
+int
+rte_telemetry_init(const char *runtime_dir, const char *rte_version, rte_cpuset_t *cpuset,
+		rte_log_fn log_fn, uint32_t registered_logtype);
+
+#endif /* RTE_HAS_CPUSET */
+
 #endif

Signature · 1 line

2.27.0

Thomas Monjalon

Applied

2021-03-25 16:36 UTC #18 lore raw

In reply to Bruce Richardson

25/03/2021 14:57, Bruce Richardson:

Quoted 10 lines

This set adds support for using the regular rte_log functions from the telemetry library; avoiding circular dependencies by having EAL register the telemetry library itself and then passing the required handles to that library as part of the telemetry_init call.
Beyond this change, the other three patches are cleanups to ensure that all internal functions are clearly separate from the public APIs. (Patches 3 & 4 may be merged into a single one on apply, for I've kept them separate for now for clarity).

Quoted 7 lines

V2: Rebased on latest main branch.
Bruce Richardson (4):
telemetry: use rte_log for logging
telemetry: make the legacy registration function internal
telemetry: rename internal-only header file
telemetry: move init function to internal header

Applied, thanks.