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

[Morten Brørup]

> From: Robin Jarry [mailto:rjarry at redhat.com]
> Sent: Friday, 16 December 2022 11.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 at redhat.com>
> Acked-by: Morten Brørup <mb at smartsharesystems.com>
> ---
> v4 -> v5:
> 
> The callback now takes a pointer to a rte_lcore_usage structure.
> I chose not to include any API version tracking mechanism since the
> unsupported/unused fields can simply be left to zero. This is only
> telemetry after all.

ACK to this decision, with a minor clarification to avoid any misinterpretation:

The callback should not modify (i.e. zero out) unsupported/unused fields.

The caller needs to clear the structure before calling the callback - because the callback might not use the updated size of the structure, if the application was written for an older DPDK version with a smaller structure. I can see you already do this. Consider adding a comment about it in the code.

[...]

>  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];
> +	struct rte_lcore_usage usage;
> +	rte_lcore_usage_cb usage_cb;
>  	const char *role;
>  	FILE *f = arg;
>  	int ret;
> @@ -446,11 +457,19 @@ lcore_dump_cb(unsigned int lcore_id, void *arg)
>  		break;
>  	}
> 
> +	memset(&usage, 0, sizeof(usage));

I would move this memset() inside the below if-block.

> +	usage_str[0] = '\0';
> +	usage_cb = lcore_usage_cb;
> +	if (usage_cb != NULL && usage_cb(lcore_id, &usage) == 0) {

Move memset() inside here, and add comment:

+ /* The application's callback may not set all the fields in the structure, so clear it here. */
+ memset(&usage, 0, sizeof(usage));

> +		snprintf(usage_str, sizeof(usage_str), ", busy cycles
> %"PRIu64"/%"PRIu64,
> +			usage.busy_cycles, usage.total_cycles);
> +	}

[...]

> @@ -522,6 +543,12 @@ 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);
> +	memset(&usage, 0, sizeof(usage));
> +	usage_cb = lcore_usage_cb;
> +	if (usage_cb != NULL && usage_cb(lcore_id, &usage) == 0) {

Same comment as above: Move memset() inside here, and add a comment about why the structure is cleared here.

> +		rte_tel_data_add_dict_u64(info->d, "busy_cycles",
> usage.busy_cycles);
> +		rte_tel_data_add_dict_u64(info->d, "total_cycles",
> usage.total_cycles);
> +	}
> 
>  	return 0;
>  }
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 6938c3fd7b81..a92313577355 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -328,6 +328,41 @@ typedef int (*rte_lcore_iterate_cb)(unsigned int
> lcore_id, void *arg);
>  int
>  rte_lcore_iterate(rte_lcore_iterate_cb cb, void *arg);
> 
> +/**
> + * CPU usage statistics.
> + */
> +struct rte_lcore_usage {
> +	uint64_t total_cycles;
> +	/**< The total amount of time since application start, in TSC
> cycles. */
> +	uint64_t busy_cycles;
> +	/**< The amount of busy time since application start, in TSC
> cycles. */
> +};
> +
> +/**
> + * Callback to allow applications to report CPU usage.
> + *
> + * @param [in] lcore_id
> + *   The lcore to consider.
> + * @param [out] usage
> + *   Counters representing this lcore usage. This can never be NULL.
> + * @return
> + *   - 0 if fields in usage were updated successfully. The fields that
> the
> + *       application does not support should be left to their default
> value.

"should be left to their default value." -> "must not be modified."

> + *   - a negative value if the information is not available or if any
> error occurred.
> + */
> +typedef int (*rte_lcore_usage_cb)(unsigned int lcore_id, struct
> rte_lcore_usage *usage);