diff mbox series

[2/4] eal: allow applications to report their cpu usage

Message ID 20221207162124.769994-3-rjarry@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series lcore telemetry improvements |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Robin Jarry Dec. 7, 2022, 4:21 p.m. UTC 
  Allow applications to register a callback that will be invoked in
rte_lcore_dump() and when requesting lcore info in the telemetry API.

The callback is expected to return the number of TSC cycles that have
passed since application start and the number of these cycles that were
spent doing busy work.

Signed-off-by: Robin Jarry <rjarry@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
v3 -> v4: Changed nomenclature: CPU cycles -> TSC cycles

 lib/eal/common/eal_common_lcore.c | 35 ++++++++++++++++++++++++++++---
 lib/eal/include/rte_lcore.h       | 29 +++++++++++++++++++++++++
 lib/eal/version.map               |  1 +
 3 files changed, 62 insertions(+), 3 deletions(-)

Comments

Robin Jarry Dec. 13, 2022, 3:49 p.m. UTC | #1 
Robin Jarry, Dec 07, 2022 at 17:21:
> Allow applications to register a callback that will be invoked in
> rte_lcore_dump() and when requesting lcore info in the telemetry API.
>
> The callback is expected to return the number of TSC cycles that have
> passed since application start and the number of these cycles that were
> spent doing busy work.
>
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v3 -> v4: Changed nomenclature: CPU cycles -> TSC cycles

As you may have noticed, I forgot to add -v4 for that iteration...

> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 6938c3fd7b81..df7f0a8e07c6 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -328,6 +328,35 @@ typedef int (*rte_lcore_iterate_cb)(unsigned int lcore_id, void *arg);
>  int
>  rte_lcore_iterate(rte_lcore_iterate_cb cb, void *arg);
>  
> +/**
> + * Callback to allow applications to report CPU usage.
> + *
> + * @param [in] lcore_id
> + *   The lcore to consider.
> + * @param [out] busy_cycles
> + *   The amount of busy time since application start, in TSC cycles.
> + * @param [out] total_cycles
> + *   The total amount of time since application start, in TSC cycles.
> + * @return
> + *   - 0 if both busy and total were set correctly.
> + *   - a negative value if the information is not available or if any error occurred.
> + */
> +typedef int (*rte_lcore_usage_cb)(
> +	unsigned int lcore_id, uint64_t *busy_cycles, uint64_t *total_cycles);

Instead of two uint64_t pointers, I was thinking a better approach would
be to pass a pointer to a struct containing these two fields. That way
it leaves room for adding more counters if need be. And do so without
breaking the ABI.

Thoughts?
Morten Brørup Dec. 13, 2022, 4:39 p.m. UTC | #2 
> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Tuesday, 13 December 2022 16.50
> 
> Robin Jarry, Dec 07, 2022 at 17:21:
> > Allow applications to register a callback that will be invoked in
> > rte_lcore_dump() and when requesting lcore info in the telemetry API.
> >
> > The callback is expected to return the number of TSC cycles that have
> > passed since application start and the number of these cycles that
> were
> > spent doing busy work.
> >
> > Signed-off-by: Robin Jarry <rjarry@redhat.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v3 -> v4: Changed nomenclature: CPU cycles -> TSC cycles
> 
> As you may have noticed, I forgot to add -v4 for that iteration...
> 
> > diff --git a/lib/eal/include/rte_lcore.h
> b/lib/eal/include/rte_lcore.h
> > index 6938c3fd7b81..df7f0a8e07c6 100644
> > --- a/lib/eal/include/rte_lcore.h
> > +++ b/lib/eal/include/rte_lcore.h
> > @@ -328,6 +328,35 @@ typedef int (*rte_lcore_iterate_cb)(unsigned int
> lcore_id, void *arg);
> >  int
> >  rte_lcore_iterate(rte_lcore_iterate_cb cb, void *arg);
> >
> > +/**
> > + * Callback to allow applications to report CPU usage.
> > + *
> > + * @param [in] lcore_id
> > + *   The lcore to consider.
> > + * @param [out] busy_cycles
> > + *   The amount of busy time since application start, in TSC cycles.
> > + * @param [out] total_cycles
> > + *   The total amount of time since application start, in TSC
> cycles.
> > + * @return
> > + *   - 0 if both busy and total were set correctly.
> > + *   - a negative value if the information is not available or if
> any error occurred.
> > + */
> > +typedef int (*rte_lcore_usage_cb)(
> > +	unsigned int lcore_id, uint64_t *busy_cycles, uint64_t
> *total_cycles);
> 
> Instead of two uint64_t pointers, I was thinking a better approach
> would
> be to pass a pointer to a struct containing these two fields. That way
> it leaves room for adding more counters if need be. And do so without
> breaking the ABI.
> 
> Thoughts?

I like the idea.

For compatibility between newer DPDK libraries (with more fields) and older applications, the callback should return an indication of how much of the structure it has filled, so DPDK knows that some fields are unfilled.

The simplest method would be that the callback returns the number of bytes of the structure filled instead of 0 on success. However, that would not allow for holes in the returned structure.

Alternatively, a bitfield can be the first field in the structure, each bit representing a data field in the structure. That would allow flexibility to fill any of up to 64 fields. So with total_cycles and busy_cycles as data fields, the returned structure would contain e.g. {3, 1000, 900}. (As a personal preference, I would put total_cycles before busy_cycles in such a structure.)

And I'm not saying that fields must be uint64_t; they can be any size.

On the other hand, I might be suggesting too much flexibility with the bitfield proposal. Perhaps the simple method suffices. And perhaps only uint64_t fields suffice.

-Morten
Tyler Retzlaff Dec. 13, 2022, 5:45 p.m. UTC | #3 
On Tue, Dec 13, 2022 at 04:49:31PM +0100, Robin Jarry wrote:
> Robin Jarry, Dec 07, 2022 at 17:21:
> > Allow applications to register a callback that will be invoked in
> > rte_lcore_dump() and when requesting lcore info in the telemetry API.
> >
> > The callback is expected to return the number of TSC cycles that have
> > passed since application start and the number of these cycles that were
> > spent doing busy work.
> >
> > Signed-off-by: Robin Jarry <rjarry@redhat.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v3 -> v4: Changed nomenclature: CPU cycles -> TSC cycles
> 
> As you may have noticed, I forgot to add -v4 for that iteration...
> 
> > diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> > index 6938c3fd7b81..df7f0a8e07c6 100644
> > --- a/lib/eal/include/rte_lcore.h
> > +++ b/lib/eal/include/rte_lcore.h
> > @@ -328,6 +328,35 @@ typedef int (*rte_lcore_iterate_cb)(unsigned int lcore_id, void *arg);
> >  int
> >  rte_lcore_iterate(rte_lcore_iterate_cb cb, void *arg);
> >  
> > +/**
> > + * Callback to allow applications to report CPU usage.
> > + *
> > + * @param [in] lcore_id
> > + *   The lcore to consider.
> > + * @param [out] busy_cycles
> > + *   The amount of busy time since application start, in TSC cycles.
> > + * @param [out] total_cycles
> > + *   The total amount of time since application start, in TSC cycles.
> > + * @return
> > + *   - 0 if both busy and total were set correctly.
> > + *   - a negative value if the information is not available or if any error occurred.
> > + */
> > +typedef int (*rte_lcore_usage_cb)(
> > +	unsigned int lcore_id, uint64_t *busy_cycles, uint64_t *total_cycles);
> 
> Instead of two uint64_t pointers, I was thinking a better approach would
> be to pass a pointer to a struct containing these two fields. That way
> it leaves room for adding more counters if need be. And do so without
> breaking the ABI.
> 
> Thoughts?

yes, please.
diff mbox series

Patch

diff --git a/lib/eal/common/eal_common_lcore.c b/lib/eal/common/eal_common_lcore.c
index 16548977dce8..23717abf6530 100644
--- a/lib/eal/common/eal_common_lcore.c
+++ b/lib/eal/common/eal_common_lcore.c
@@ -2,6 +2,7 @@ 
  * Copyright(c) 2010-2014 Intel Corporation
  */
 
+#include <inttypes.h>
 #include <stdlib.h>
 #include <string.h>
 
@@ -422,11 +423,21 @@  rte_lcore_iterate(rte_lcore_iterate_cb cb, void *arg)
 	return ret;
 }
 
+static rte_lcore_usage_cb lcore_usage_cb;
+
+void
+rte_lcore_register_usage_cb(rte_lcore_usage_cb cb)
+{
+	lcore_usage_cb = cb;
+}
+
 static int
 lcore_dump_cb(unsigned int lcore_id, void *arg)
 {
 	struct rte_config *cfg = rte_eal_get_configuration();
-	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
+	char cpuset[RTE_CPU_AFFINITY_STR_LEN], usage_str[256];
+	uint64_t busy_cycles, total_cycles;
+	rte_lcore_usage_cb usage_cb;
 	const char *role;
 	FILE *f = arg;
 	int ret;
@@ -446,11 +457,20 @@  lcore_dump_cb(unsigned int lcore_id, void *arg)
 		break;
 	}
 
+	busy_cycles = 0;
+	total_cycles = 0;
+	usage_str[0] = '\0';
+	usage_cb = lcore_usage_cb;
+	if (usage_cb != NULL && usage_cb(lcore_id, &busy_cycles, &total_cycles) == 0) {
+		snprintf(usage_str, sizeof(usage_str), ", busy cycles %"PRIu64"/%"PRIu64,
+			busy_cycles, total_cycles);
+	}
 	ret = eal_thread_dump_affinity(&lcore_config[lcore_id].cpuset, cpuset,
 		sizeof(cpuset));
-	fprintf(f, "lcore %u, socket %u, role %s, cpuset %s%s\n", lcore_id,
+	fprintf(f, "lcore %u, socket %u, role %s, cpuset %s%s%s\n", lcore_id,
 		rte_lcore_to_socket_id(lcore_id), role, cpuset,
-		ret == 0 ? "" : "...");
+		ret == 0 ? "" : "...", usage_str);
+
 	return 0;
 }
 
@@ -489,7 +509,9 @@  lcore_telemetry_info_cb(unsigned int lcore_id, void *arg)
 {
 	struct lcore_telemetry_info *info = arg;
 	struct rte_config *cfg = rte_eal_get_configuration();
+	uint64_t busy_cycles, total_cycles;
 	struct rte_tel_data *cpuset;
+	rte_lcore_usage_cb usage_cb;
 	const char *role;
 	unsigned int cpu;
 
@@ -522,6 +544,13 @@  lcore_telemetry_info_cb(unsigned int lcore_id, void *arg)
 		if (CPU_ISSET(cpu, &lcore_config[lcore_id].cpuset))
 			rte_tel_data_add_array_int(cpuset, cpu);
 	rte_tel_data_add_dict_container(info->d, "cpuset", cpuset, 0);
+	busy_cycles = 0;
+	total_cycles = 0;
+	usage_cb = lcore_usage_cb;
+	if (usage_cb != NULL && usage_cb(lcore_id, &busy_cycles, &total_cycles) == 0) {
+		rte_tel_data_add_dict_u64(info->d, "busy_cycles", busy_cycles);
+		rte_tel_data_add_dict_u64(info->d, "total_cycles", total_cycles);
+	}
 
 	return 0;
 }
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 6938c3fd7b81..df7f0a8e07c6 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -328,6 +328,35 @@  typedef int (*rte_lcore_iterate_cb)(unsigned int lcore_id, void *arg);
 int
 rte_lcore_iterate(rte_lcore_iterate_cb cb, void *arg);
 
+/**
+ * Callback to allow applications to report CPU usage.
+ *
+ * @param [in] lcore_id
+ *   The lcore to consider.
+ * @param [out] busy_cycles
+ *   The amount of busy time since application start, in TSC cycles.
+ * @param [out] total_cycles
+ *   The total amount of time since application start, in TSC cycles.
+ * @return
+ *   - 0 if both busy and total were set correctly.
+ *   - a negative value if the information is not available or if any error occurred.
+ */
+typedef int (*rte_lcore_usage_cb)(
+	unsigned int lcore_id, uint64_t *busy_cycles, uint64_t *total_cycles);
+
+/**
+ * Register a callback from an application to be called in rte_lcore_dump()
+ * and the /eal/lcore/info telemetry endpoint handler.
+ *
+ * Applications are expected to report the amount of busy and total TSC cycles
+ * since their startup.
+ *
+ * @param cb
+ *   The callback function.
+ */
+__rte_experimental
+void rte_lcore_register_usage_cb(rte_lcore_usage_cb cb);
+
 /**
  * List all lcores.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7dc985..30fd216a12ea 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,7 @@  EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+	rte_lcore_register_usage_cb;
 };
 
 INTERNAL {