> From: Robin Jarry [mailto:rja...@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 <rja...@redhat.com>
> Acked-by: Morten Brørup <m...@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);
