Re: [dpdk-dev] [PATCH v1] examples/l3fwd-power: add telemetry mode support

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 17-May-19 7:17 PM, Reshma Pattan wrote:
> Add new telemetry mode support for l3fwd-power.
> This is a standalone mode, in this mode l3fwd-power
> does simple l3fwding along with calculating
> empty polls, full polls, and busy percentage for
> each forwarding core. The aggregation of these
> values of all cores is reported as application
> level telemetry to metric library for every 10ms from the
> master core.
> 
> The busy percentage is calculated by recording the poll_count
> and when the count reaches a defined value the total
> cycles it took is measured and compared with minimum and maximum
> reference cycles and busy rate is set according to either 0% or
> 50% or 100%.
> 
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> ---

<snip>

> +			poll_count = 0;
> +			prev_tel_tsc = cur_tsc;
> +			/* update stats for telemetry */
> +			rte_spinlock_lock(&stats[lcore_id].telemetry_lock);
> +			stats[lcore_id].ep_nep[0] = ep_nep[0];
> +			stats[lcore_id].ep_nep[1] = ep_nep[1];
> +			stats[lcore_id].fp_nfp[0] = fp_nfp[0];
> +			stats[lcore_id].fp_nfp[1] = fp_nfp[1];
> +			stats[lcore_id].br = br;
> +			rte_spinlock_unlock(&stats[lcore_id].telemetry_lock);

Locking here seems relatively rare (per-lcore and once every N polls), 
but any locking on a hotpath makes me nervous. What is the current 
performance impact of this? Should we bother improving?

> +		}
> +	}
> +
> +	return 0;
> +}
> +/* main processing loop */
> +static int
>   main_empty_poll_loop(__attribute__((unused)) void *dummy)
>   {
>   	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
> @@ -1274,7 +1455,9 @@ print_usage(const char *prgname)
>   		" which max packet len is PKTLEN in decimal (64-9600)\n"
>   		"  --parse-ptype: parse packet type by software\n"
>   		"  --empty-poll: enable empty poll detection"
> -		" follow (training_flag, high_threshold, med_threshold)\n",
> +		" follow (training_flag, high_threshold, med_threshold)\n"
> +		" --telemetry: enable telemetry mode, to upadte"

Update :)

> +		" empty polls, full polls, and core busyness to telemetry\n",

I don't particularly like the term "busyness". Load? Workload? I'm OK 
with keeping as is though, just a personal preference :)

>   		prgname);
>   }
>   
> @@ -1417,6 +1600,7 @@ parse_ep_config(const char *q_arg)
>   
>   }
>   #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
> +#define CMD_LINE_OPT_TELEMETRY "telemetry"

<snip>

>   
>   			if (!strncmp(lgopts[option_index].name,
> @@ -1869,6 +2068,52 @@ init_power_library(void)
>   	return ret;
>   }
>   static void
> +update_telemetry(__attribute__((unused)) struct rte_timer *tim,
> +		__attribute__((unused)) void *arg)
> +{

I would question the need to put telemetry on a high precision 10ms 
timer. Is there any reason why we cannot gather telemetry, say, once 
every 100ms, and why we cannot do so from interrupt thread using alarm 
API? Using high-precision timer API here seems like an overkill.

> +	unsigned int lcore_id = rte_lcore_id();
> +	struct lcore_conf *qconf;
> +	uint64_t app_eps = 0, app_fps = 0, app_br = 0;
> +	uint64_t values[3] = {0};
> +	int ret;
> +	uint64_t count = 0;
> +
> +	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +		qconf = &lcore_conf[lcore_id];
> +		if (qconf->n_rx_queue != 0) {

Perhaps just to an early exit? It will avoid having extra indentation 
level :)

e.g.

if (qconf->n_rx_queue == 0)
    continue;
<rest of the code>

> +			count++;
> +			rte_spinlock_lock(&stats[lcore_id].telemetry_lock);
> +			app_eps += stats[lcore_id].ep_nep[1];
> +			app_fps += stats[lcore_id].fp_nfp[1];
> +			app_br += stats[lcore_id].br;
> +			rte_spinlock_unlock(&stats[lcore_id].telemetry_lock);
> +		}

<snip>

>   	/* launch per-lcore init on every lcore */
> -	if (empty_poll_on == false) {
> +	if (app_mode == APP_MODE_LEGACY) {
>   		rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
> -	} else {
> +	} else if (app_mode == APP_MODE_EMPTY_POLL) {
>   		empty_poll_stop = false;
>   		rte_eal_mp_remote_launch(main_empty_poll_loop, NULL,
>   				SKIP_MASTER);
> +	} else {
> +		/* Init metrics library */
> +		rte_metrics_init(rte_socket_id());
> +		/** Register latency stats with stats library */

Latency? Probably a copy-paste error?


-- 
Thanks,
Anatoly