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

[Morten Brørup]

> From: Kevin Laatz [mailto:kevin.laatz at intel.com]
> Sent: Wednesday, 18 January 2023 12.03
> 
> On 18/01/2023 10:21, Morten Brørup wrote:
> >> From: Kevin Laatz [mailto:kevin.laatz at intel.com]
> >>
> >> 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 at redhat.com>
> >>> Acked-by: Morten Brørup <mb at 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
> >
> > {

I forgot the index array:

"lcore_id": [1,6,7],

> > "total_cycles": [1234, 1234, 1234],
> > "usage_cycles": [567, 678, 789]
> > }
> 
> +1, this would also work nicely and allows for extension in future
> without flooding with endpoints.

Our applications do something somewhat similar, i.e. use JSON arrays for performance (and bandwidth conservation) reasons - so I can confirm that the concept works in production.

But come to think of it, your array suggestion seems more like an additional feature, which belongs in another patch. We should not hold back Robin's patch series due to feature creep. :-)

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