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

  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 CPU 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>
---
v2 -> v3:

- Copied callback to local variable to guard against (unlikely) races.
- Used != NULL convention to test if callback is defined.
- Fixed typo in doc string.
- Did not add a % value in rte_lcore_dump() as its use would be very
  limited.

v1 -> v2:

Changed the approach based on Morten's review: the callback is now
expected to report the total number of cycles since application start
and the amount of these cycles that were spent doing busy work. This
will give more flexibility in external monitoring tools to decide the
sample period to compute busyness ratio.

 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(-)
  

On 2022-11-29 16:33, Robin Jarry wrote:
> 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 CPU 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>
> ---
> v2 -> v3:
> 
> - Copied callback to local variable to guard against (unlikely) races.
> - Used != NULL convention to test if callback is defined.
> - Fixed typo in doc string.
> - Did not add a % value in rte_lcore_dump() as its use would be very
>    limited.
> 
> v1 -> v2:
> 
> Changed the approach based on Morten's review: the callback is now
> expected to report the total number of cycles since application start
> and the amount of these cycles that were spent doing busy work. This
> will give more flexibility in external monitoring tools to decide the
> sample period to compute busyness ratio.
> 
>   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(-)
> 
> 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..0552e6f44142 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 number of busy CPU cycles since the application start.

Wouldn't it be more appropriate to describe it as "TSC cycles", instead 
of "CPU cycles"? Or technically "The amount of busy time since 
application start, in TSC cycles".

Reporting actual CPU core cycles spent is not possible, in the general case.

> + * @param [out] total_cycles
> + *   The total number of CPU cycles since the application start.
> + * @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 CPU 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 {
  

Hi Mattias,

Mattias Rönnblom, Nov 29, 2022 at 17:10:
> > + * @param [out] busy_cycles
> > + *   The number of busy CPU cycles since the application start.
>
> Wouldn't it be more appropriate to describe it as "TSC cycles", instead 
> of "CPU cycles"? Or technically "The amount of busy time since 
> application start, in TSC cycles".
>
> Reporting actual CPU core cycles spent is not possible, in the general case.

Well, technically, these "cycles" could mean anything. You could report
the number of microseconds, Joules, whatever. The important information
is the ratio compared to the total number of "cycles". Perhaps Morten's
suggestion (units) would be more precise, but I fear it would also be
more confusing.

I don't mind adding the specific TSC cycles nomenclature if there is
a consensus.

Cheers
  

> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Wednesday, 7 December 2022 12.01
> 
> Hi Mattias,
> 
> Mattias Rönnblom, Nov 29, 2022 at 17:10:
> > > + * @param [out] busy_cycles
> > > + *   The number of busy CPU cycles since the application start.
> >
> > Wouldn't it be more appropriate to describe it as "TSC cycles",
> instead
> > of "CPU cycles"? Or technically "The amount of busy time since
> > application start, in TSC cycles".
> >
> > Reporting actual CPU core cycles spent is not possible, in the
> general case.
> 
> Well, technically, these "cycles" could mean anything. You could report
> the number of microseconds, Joules, whatever. The important information
> is the ratio compared to the total number of "cycles". Perhaps Morten's
> suggestion (units) would be more precise, but I fear it would also be
> more confusing.
> 
> I don't mind adding the specific TSC cycles nomenclature if there is
> a consensus.

Let's stick with TSC cycles. As you mention yourself, sometimes clarity is better than abstraction.

Having a fixed unit of measure also makes it easier to use in DPDK libraries and wherever, in the future.

-Morten