Re: [PATCH v1 1/2] eal: add lcore busyness telemetry

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Jul 15, 2022 at 6:42 PM Anatoly Burakov
<anatoly.burakov@intel.com> wrote:
>
> Currently, there is no way to measure lcore busyness in a passive way,
> without any modifications to the application. This patch adds a new EAL
> API that will be able to passively track core busyness.
>
> The busyness is calculated by relying on the fact that most DPDK API's
> will poll for packets. Empty polls can be counted as "idle", while
> non-empty polls can be counted as busy. To measure lcore busyness, we
> simply call the telemetry timestamping function with the number of polls
> a particular code section has processed, and count the number of cycles
> we've spent processing empty bursts. The more empty bursts we encounter,
> the less cycles we spend in "busy" state, and the less core busyness
> will be reported.
>
> In order for all of the above to work without modifications to the
> application, the library code needs to be instrumented with calls to
> the lcore telemetry busyness timestamping function. The following parts
> of DPDK are instrumented with lcore telemetry calls:
>
> - All major driver API's:
>   - ethdev
>   - cryptodev
>   - compressdev
>   - regexdev
>   - bbdev
>   - rawdev
>   - eventdev
>   - dmadev
> - Some additional libraries:
>   - ring
>   - distributor
>
> To avoid performance impact from having lcore telemetry support, a
> global variable is exported by EAL, and a call to timestamping function
> is wrapped into a macro, so that whenever telemetry is disabled, it only
> takes one additional branch and no function calls are performed. It is
> also possible to disable it at compile time by commenting out
> RTE_LCORE_BUSYNESS from build config.
>
> This patch also adds a telemetry endpoint to report lcore busyness, as
> well as telemetry endpoints to enable/disable lcore telemetry.
>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> Signed-off-by: Conor Walsh <conor.walsh@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>


It is a good feature. Thanks for this work.


> ---
>
> Notes:
>     We did a couple of quick smoke tests to see if this patch causes any performance
>     degradation, and it seemed to have none that we could measure. Telemetry can be
>     disabled at compile time via a config option, while at runtime it can be
>     disabled, seemingly at a cost of one additional branch.
>
>     That said, our benchmarking efforts were admittedly not very rigorous, so
>     comments welcome!

>
> diff --git a/config/rte_config.h b/config/rte_config.h
> index 46549cb062..583cb6f7a5 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -39,6 +39,8 @@
>  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
>  #define RTE_BACKTRACE 1
>  #define RTE_MAX_VFIO_CONTAINERS 64
> +#define RTE_LCORE_BUSYNESS 1

Please don't enable debug features in fastpath as default.

> +#define RTE_LCORE_BUSYNESS_PERIOD 4000000ULL
>
> +
> +#include <unistd.h>
> +#include <limits.h>
> +#include <string.h>
> +
> +#include <rte_common.h>
> +#include <rte_cycles.h>
> +#include <rte_errno.h>
> +#include <rte_lcore.h>
> +
> +#ifdef RTE_LCORE_BUSYNESS

This clutter may not be required. Let it compile all cases.

> +#include <rte_telemetry.h>
> +#endif
> +
> +int __rte_lcore_telemetry_enabled;
> +
> +#ifdef RTE_LCORE_BUSYNESS
> +
> +struct lcore_telemetry {
> +       int busyness;
> +       /**< Calculated busyness (gets set/returned by the API) */
> +       int raw_busyness;
> +       /**< Calculated busyness times 100. */
> +       uint64_t interval_ts;
> +       /**< when previous telemetry interval started */
> +       uint64_t empty_cycles;
> +       /**< empty cycle count since last interval */
> +       uint64_t last_poll_ts;
> +       /**< last poll timestamp */
> +       bool last_empty;
> +       /**< if last poll was empty */
> +       unsigned int contig_poll_cnt;
> +       /**< contiguous (always empty/non empty) poll counter */
> +} __rte_cache_aligned;
> +static struct lcore_telemetry telemetry_data[RTE_MAX_LCORE];

Allocate this from hugepage.

> +
> +void __rte_lcore_telemetry_timestamp(uint16_t nb_rx)
> +{
> +       const unsigned int lcore_id = rte_lcore_id();
> +       uint64_t interval_ts, empty_cycles, cur_tsc, last_poll_ts;
> +       struct lcore_telemetry *tdata = &telemetry_data[lcore_id];
> +       const bool empty = nb_rx == 0;
> +       uint64_t diff_int, diff_last;
> +       bool last_empty;
> +
> +       last_empty = tdata->last_empty;
> +
> +       /* optimization: don't do anything if status hasn't changed */
> +       if (last_empty == empty && tdata->contig_poll_cnt++ < 32)
> +               return;
> +       /* status changed or we're waiting for too long, reset counter */
> +       tdata->contig_poll_cnt = 0;
> +
> +       cur_tsc = rte_rdtsc();
> +
> +       interval_ts = tdata->interval_ts;
> +       empty_cycles = tdata->empty_cycles;
> +       last_poll_ts = tdata->last_poll_ts;
> +
> +       diff_int = cur_tsc - interval_ts;
> +       diff_last = cur_tsc - last_poll_ts;
> +
> +       /* is this the first time we're here? */
> +       if (interval_ts == 0) {
> +               tdata->busyness = LCORE_BUSYNESS_MIN;
> +               tdata->raw_busyness = 0;
> +               tdata->interval_ts = cur_tsc;
> +               tdata->empty_cycles = 0;
> +               tdata->contig_poll_cnt = 0;
> +               goto end;
> +       }
> +
> +       /* update the empty counter if we got an empty poll earlier */
> +       if (last_empty)
> +               empty_cycles += diff_last;
> +
> +       /* have we passed the interval? */
> +       if (diff_int > RTE_LCORE_BUSYNESS_PERIOD) {


I think, this function logic can be limited to just updating the
timestamp in the ring buffer,
and another control function of telemetry which runs in control core to do
heavy lifting to reduce the performance impact on fast path,

> +               int raw_busyness;
> +
> +               /* get updated busyness value */
> +               raw_busyness = calc_raw_busyness(tdata, empty_cycles, diff_int);
> +
> +               /* set a new interval, reset empty counter */
> +               tdata->interval_ts = cur_tsc;
> +               tdata->empty_cycles = 0;
> +               tdata->raw_busyness = raw_busyness;
> +               /* bring busyness back to 0..100 range, biased to round up */
> +               tdata->busyness = (raw_busyness + 50) / 100;
> +       } else
> +               /* we may have updated empty counter */
> +               tdata->empty_cycles = empty_cycles;
> +
> +end:
> +       /* update status for next poll */
> +       tdata->last_poll_ts = cur_tsc;
> +       tdata->last_empty = empty;
> +}
> +
> +#ifdef RTE_LCORE_BUSYNESS
> +#define RTE_LCORE_TELEMETRY_TIMESTAMP(nb_rx)                    \
> +       do {                                                    \
> +               if (__rte_lcore_telemetry_enabled)              \

I think, rather than reading memory, Like Linux perf infrastructure,
we can patch up the instruction steam as NOP vs Timestamp capture
function.


Also instead of changing all libraries, Maybe we can use
"-finstrument-functions".
and just mark the function with attribute.

Just 2c.

> +                       __rte_lcore_telemetry_timestamp(nb_rx); \
> +       } while (0)
> +#else
> +#define RTE_LCORE_TELEMETRY_TIMESTAMP(nb_rx) \
> +       while (0)
> +#endif
> +