RE: [PATCH v5 1/4] eal: add lcore info in telemetry

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Kevin Laatz [mailto:kevin.laatz@intel.com]
> Sent: Wednesday, 18 January 2023 10.42
> To: Robin Jarry; dev@dpdk.org
> 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 <rjarry@redhat.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---


> 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.