[v8,2/5] eal: report applications lcore usage

  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@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>
---

Notes:
    v7 -> v8: no change

 doc/guides/rel_notes/release_23_03.rst |  7 ++++
 lib/eal/common/eal_common_lcore.c      | 48 ++++++++++++++++++++++++--
 lib/eal/include/rte_lcore.h            | 35 +++++++++++++++++++
 lib/eal/version.map                    |  1 +
 4 files changed, 89 insertions(+), 2 deletions(-)
  

Hi Robin,

On 2023/2/2 21:43, Robin Jarry wrote:
> 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.
> 

The 'record_burst_stats' also important for performance tune.

Is it possible to incorporate it into this framework?
  

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Monday, 6 February 2023 05.00
> 
> Hi Robin,
> 
> On 2023/2/2 21:43, Robin Jarry wrote:
> > 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.
> >
> 
> The 'record_burst_stats' also important for performance tune.

While I agree that burst size spread is important for comparing CPU usage and capacity, the 'record_burst_stats' is a test_pmd specific feature. Applications may implement something different.

> 
> Is it possible to incorporate it into this framework?

We all agreed on CPU usage metrics for this patch, so let's limit ourselves this time.

If we want more performance metrics, such as RX and TX burst size spread, they can be proposed and discussed in a separate RFC. It might trigger a lot of discussion, like the original lcore busyness patch that inspired this patch series.
  

Morten Brørup, Feb 06, 2023 at 08:36:
> > The 'record_burst_stats' also important for performance tune.
>
> While I agree that burst size spread is important for comparing CPU 
> usage and capacity, the 'record_burst_stats' is a test_pmd specific 
> feature. Applications may implement something different.
>
> > Is it possible to incorporate it into this framework?
>
> We all agreed on CPU usage metrics for this patch, so let's limit 
> ourselves this time.
>
> If we want more performance metrics, such as RX and TX burst size 
> spread, they can be proposed and discussed in a separate RFC. It might 
> trigger a lot of discussion, like the original lcore busyness patch 
> that inspired this patch series.

I agree with Morten here. This framework should only allow reporting 
statistics that are not specific to any DPDK application.
  

Hello Robin,

On Thu, Feb 2, 2023 at 2:44 PM Robin Jarry <rjarry@redhat.com> wrote:
>
> 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@redhat.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>
> Notes:
>     v7 -> v8: no change
>
>  doc/guides/rel_notes/release_23_03.rst |  7 ++++
>  lib/eal/common/eal_common_lcore.c      | 48 ++++++++++++++++++++++++--
>  lib/eal/include/rte_lcore.h            | 35 +++++++++++++++++++
>  lib/eal/version.map                    |  1 +
>  4 files changed, 89 insertions(+), 2 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
> index 73f5d94e143d..f407dc3df7a8 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -78,6 +78,13 @@ New Features
>      ``rte_event_dev_config::nb_single_link_event_port_queues`` parameter
>      required for eth_rx, eth_tx, crypto and timer eventdev adapters.
>
> +* **Added support for reporting lcore usage in applications.**
> +
> +  * The ``/eal/lcore/list`` and ``/eal/lcore/info`` telemetry endpoints have
> +    been added to provide information similar to ``rte_lcore_dump()``.
> +  * Applications can register a callback at startup via
> +    ``rte_lcore_register_usage_cb()`` to provide lcore usage information.
> +

EAL updates come first in RN sections.


>
>  Removed Items
>  -------------
> diff --git a/lib/eal/common/eal_common_lcore.c b/lib/eal/common/eal_common_lcore.c
> index f53fc17b4d04..bbb734098b42 100644
> --- a/lib/eal/common/eal_common_lcore.c
> +++ b/lib/eal/common/eal_common_lcore.c
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2010-2014 Intel Corporation
>   */
>
> +#include <inttypes.h>
>  #include <stdlib.h>
>  #include <string.h>
>
> @@ -437,20 +438,49 @@ lcore_role_str(enum rte_lcore_role_t role)
>         }
>  }
>
> +static rte_lcore_usage_cb lcore_usage_cb;
> +
> +void
> +rte_lcore_register_usage_cb(rte_lcore_usage_cb cb)
> +{
> +       lcore_usage_cb = cb;
> +}
> +
>  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];
> +       struct rte_lcore_usage usage;
> +       rte_lcore_usage_cb usage_cb;
> +       char *usage_str = NULL;
>         FILE *f = arg;
>         int ret;
>
> +       /* The callback may not set all the fields in the structure, so clear it here. */
> +       memset(&usage, 0, sizeof(usage));
> +       /*
> +        * Guard against concurrent modification of lcore_usage_cb.
> +        * rte_lcore_register_usage_cb() should only be called once at application init
> +        * but nothing prevents and application to reset the callback to NULL.

This is copy/paste a few times, only commenting here:
"prevents an* application from* resetting* the callback to NULL"


> +        */
> +       usage_cb = lcore_usage_cb;
> +       if (usage_cb != NULL && usage_cb(lcore_id, &usage) == 0) {
> +               if (asprintf(&usage_str, ", busy cycles %"PRIu64"/%"PRIu64,
> +                               usage.busy_cycles, usage.total_cycles) < 0) {
> +                       return -ENOMEM;
> +               }
> +       }
>         ret = eal_thread_dump_affinity(&lcore_config[lcore_id].cpuset, cpuset,
>                 sizeof(cpuset));
> -       fprintf(f, "lcore %u, socket %u, role %s, cpuset %s%s\n", lcore_id,
> +       fprintf(f, "lcore %u, socket %u, role %s, cpuset %s%s%s\n", lcore_id,
>                 rte_lcore_to_socket_id(lcore_id),
>                 lcore_role_str(cfg->lcore_role[lcore_id]),
> -               cpuset, ret == 0 ? "" : "...");
> +               cpuset, ret == 0 ? "" : "...",
> +               usage_str ? usage_str : "");

usage_str != NULL

> +
> +       free(usage_str);
> +
>         return 0;
>  }
>
> @@ -489,7 +519,9 @@ lcore_telemetry_info_cb(unsigned int lcore_id, void *arg)
>  {
>         struct rte_config *cfg = rte_eal_get_configuration();
>         struct lcore_telemetry_info *info = arg;
> +       struct rte_lcore_usage usage;
>         struct rte_tel_data *cpuset;
> +       rte_lcore_usage_cb usage_cb;
>         unsigned int cpu;
>
>         if (info->lcore_id != lcore_id)
> @@ -508,6 +540,18 @@ lcore_telemetry_info_cb(unsigned int lcore_id, void *arg)
>                         rte_tel_data_add_array_int(cpuset, cpu);
>         }
>         rte_tel_data_add_dict_container(info->d, "cpuset", cpuset, 0);
> +       /* The callback may not set all the fields in the structure, so clear it here. */
> +       memset(&usage, 0, sizeof(usage));
> +       /*
> +        * Guard against concurrent modification of lcore_usage_cb.
> +        * rte_lcore_register_usage_cb() should only be called once at application init
> +        * but nothing prevents and application to reset the callback to NULL.
> +        */
> +       usage_cb = lcore_usage_cb;
> +       if (usage_cb != NULL && usage_cb(lcore_id, &usage) == 0) {
> +               rte_tel_data_add_dict_u64(info->d, "total_cycles", usage.total_cycles);
> +               rte_tel_data_add_dict_u64(info->d, "busy_cycles", usage.busy_cycles);
> +       }
>
>         return 0;
>  }
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 9c7865052100..b1c8afb05d28 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);
>
> +/**
> + * lcore usage statistics.
> + */
> +struct rte_lcore_usage {
> +       /** The total amount of time since application start, in TSC cycles. */
> +       uint64_t total_cycles;

This code comment needs some clarification.

What is this "total amount of time"?
"application start" is ambiguous.
EAL lcore threads are not created/started by the application itself,
so the application has no idea of the time the lcore/threads were
created.

I would describe as:
/** The total amount of time that the application has been running on
this lcore, in TSC cycles. */

Is it acceptable to you?


> +       /** The amount of busy time since application start, in TSC cycles. */
> +       uint64_t busy_cycles;

And here:
/** The amount of time the application was busy, handling some
workload on this lcore, in TSC cycles. */


> +};
> +
> +/**
> + * Callback to allow applications to report lcore 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 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);
> +
> +/**

Missing a experimental banner:
 * @warning
 * @b EXPERIMENTAL: this API may change without prior notice.


> + * Register a callback from an application to be called in rte_lcore_dump() and
> + * the /eal/lcore/info telemetry endpoint handler. Applications are expected to
> + * report lcore usage statistics via this callback.
> + *
> + * @param cb
> + *   The callback function.
> + */
> +__rte_experimental
> +void rte_lcore_register_usage_cb(rte_lcore_usage_cb cb);
> +
>  /**
>   * List all lcores.
>   *
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index 6523102157e2..1f70caac7b9c 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -442,6 +442,7 @@ EXPERIMENTAL {
>
>         # added in 23.03
>         rte_thread_set_name;
> +       rte_lcore_register_usage_cb;

Alphabetical order.


>  };
>
>  INTERNAL {
> --
> 2.39.1
>
  

David Marchand, Feb 06, 2023 at 09:48:
> > +struct rte_lcore_usage {
> > +       /** The total amount of time since application start, in TSC cycles. */
> > +       uint64_t total_cycles;
>
> This code comment needs some clarification.
>
> What is this "total amount of time"?
> "application start" is ambiguous.
> EAL lcore threads are not created/started by the application itself,
> so the application has no idea of the time the lcore/threads were
> created.
>
> I would describe as:
> /** The total amount of time that the application has been running on
> this lcore, in TSC cycles. */
>
> Is it acceptable to you?

Yes, this leaves less room for interpretation.

> > +       /** The amount of busy time since application start, in TSC cycles. */
> > +       uint64_t busy_cycles;
>
> And here:
> /** The amount of time the application was busy, handling some
> workload on this lcore, in TSC cycles. */

This is in line with the total. Looks good to me.

I will address that and your other comments for v9.
  

On 2023/2/6 15:36, Morten Brørup wrote:
>> From: fengchengwen [mailto:fengchengwen@huawei.com]
>> Sent: Monday, 6 February 2023 05.00
>>
>> Hi Robin,
>>
>> On 2023/2/2 21:43, Robin Jarry wrote:
>>> 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.
>>>
>>
>> The 'record_burst_stats' also important for performance tune.
> 
> While I agree that burst size spread is important for comparing CPU usage and capacity, the 'record_burst_stats' is a test_pmd specific feature. Applications may implement something different.
> 
>>
>> Is it possible to incorporate it into this framework?
> 
> We all agreed on CPU usage metrics for this patch, so let's limit ourselves this time.
> 
> If we want more performance metrics, such as RX and TX burst size spread, they can be proposed and discussed in a separate RFC. It might trigger a lot of discussion, like the original lcore busyness patch that inspired this patch series.
> 

It's okay for a separate RFC.