> From: Kevin Laatz [mailto:[email protected]]
> Sent: Wednesday, 18 January 2023 10.42
> To: Robin Jarry; [email protected]
> Cc: Tyler Retzlaff; Morten Brørup
> Subject: Re: [PATCH v5 1/4] eal: add lcore info in telemetry
>
> On 16/12/2022 10:21, Robin Jarry wrote:
> > Report the same information than rte_lcore_dump() in the telemetry
> > API into /eal/lcore/list and /eal/lcore/info,ID.
> >
> > Example:
> >
> > --> /eal/lcore/info,3
> > {
> > "/eal/lcore/info": {
> > "lcore_id": 3,
> > "socket": 0,
> > "role": "RTE",
> > "cpuset": [
> > 3
> > ]
> > }
> > }
> >
> > Signed-off-by: Robin Jarry <[email protected]>
> > Acked-by: Morten Brørup <[email protected]>
> > ---
> Hi Robin,
>
> Thanks for taking the time to work on this. It is a good implementation
> for debug use-cases.
>
> I have 2 suggestions which would improve the usability of the data:
> 1. Could we make the lcore_id paramater on /eal/lcore/info optional?
> This would allow users to read info for all lcores in the application
> at
> once.
+1 to this suggestion.
> 2. Could we add 2 additional telemetry endpoints? One which returns an
> array of busy_cycles values and the other returns an array of
> total_cycles values. These arrays could be used in conjunction with the
> /eal/lcore/list endpoint to quickly read the usage related metrics.
> I've
> included an example diff below [1].
I prefer this done in a more generic way, see below.
>
> We have a use-case beyond debugging in which we read telemetry every
> few
> milliseconds. From a performance point of view, adding the 2 additional
> endpoints would be very beneficial.
>
> Thanks,
> Kevin
>
> [1]
>
> diff --git a/lib/eal/common/eal_common_lcore.c
> b/lib/eal/common/eal_common_lcore.c
> index 210636d21d..94ddb276c5 100644
> --- a/lib/eal/common/eal_common_lcore.c
> +++ b/lib/eal/common/eal_common_lcore.c
> @@ -569,6 +569,32 @@ handle_lcore_info(const char *cmd __rte_unused,
> const char *params, struct rte_t
> return rte_lcore_iterate(lcore_telemetry_info_cb, &info);
> }
>
> +static int
> +lcore_telemetry_busy_cycles_cb(unsigned int lcore_id, void *arg)
> +{
> + struct rte_tel_data *d = arg;
> + struct rte_lcore_usage usage;
> + rte_lcore_usage_cb usage_cb;
> + unsigned long cycles = 0;
> +
> + memset(&usage, 0, sizeof(usage));
> + usage_cb = lcore_usage_cb;
> + if (usage_cb != NULL && usage_cb(lcore_id, &usage) == 0)
> + cycles = usage.busy_cycles;
> +
> + return rte_tel_data_add_array_u64(d, cycles);
> +}
> +
> +static int
> +handle_lcore_busy_cycles(const char *cmd __rte_unused,
> + const char *params __rte_unused, struct rte_tel_data
> *d)
> +{
> + int ret = rte_tel_data_start_array(d, RTE_TEL_U64_VAL);
> + if (ret)
> + return ret;
> + return rte_lcore_iterate(lcore_telemetry_busy_cycles_cb, d);
> +}
> +
> RTE_INIT(lcore_telemetry)
> {
> rte_telemetry_register_cmd(
> @@ -577,5 +603,8 @@ RTE_INIT(lcore_telemetry)
> rte_telemetry_register_cmd(
> "/eal/lcore/info", handle_lcore_info,
> "Returns lcore info. Parameters: int
> lcore_id");
> + rte_telemetry_register_cmd(
> + "/eal/lcore/busy_cycles",
> handle_lcore_busy_cycles,
> + "List of busy cycle values. Takes no
> parameters");
> }
> #endif /* !RTE_EXEC_ENV_WINDOWS */
This should be generalized to support any named field in the rte_lcore_usage
structure.
The general path could be: /eal/lcore/usage
With optional parameter lcore_id. This should return one object (or an array of
such objects, if lcore_id is not given) with all usage fields and their values,
e.g.:
{
"lcore_id": 7,
"total_cycles": 1234,
"usage_cycles": 567
}
The paths to support the array-optimized feature you are requesting could be:
/eal/lcores/usage/total_cycles and /eal/lcores/usage/usage_cycles.
These paths should return the arrays as suggested. I only request that you
change "/lcore" to plural "/lcores" and add "/usage" to the path before the
field name in the usage table.
Alternatively, you could add a path /eal/lcores/usage_array, taking the field
names as parameters and outputting multiple arrays like this:
/eal/lcores/usage_array,total_cycles,usage_cycles
{
"total_cycles": [1234, 1234, 1234],
"usage_cycles": [567, 678, 789]
}
But I don't know if this breaks with DPDK's standard REST interface. It would
be easier if we had decided on something like OData, instead of inventing our
own.
