DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: Reshma Pattan <reshma.pattan@intel.com>, dev@dpdk.org
Cc: david.hunt@intel.com, liang.j.ma@intel.com
Subject: Re: [dpdk-dev] [PATCH v1] examples/l3fwd-power: add telemetry mode support
Date: Mon, 20 May 2019 14:17:28 +0100	[thread overview]
Message-ID: <ee4ab6bf-ddf8-cc15-3014-37b4b3818272@intel.com> (raw)
In-Reply-To: <20190517181712.12027-1-reshma.pattan@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

  reply	other threads:[~2019-05-20 13:17 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-17 18:17 Reshma Pattan
2019-05-20 13:17 ` Burakov, Anatoly [this message]
2019-05-21 14:53   ` Pattan, Reshma
2019-05-22  9:46     ` Burakov, Anatoly
2019-05-22 13:06       ` Pattan, Reshma
2019-05-22 15:16 ` Stephen Hemminger
2019-05-24  9:34 ` [dpdk-dev] [PATCH v2] " Reshma Pattan
2019-05-24 10:14   ` Burakov, Anatoly
2019-06-13 13:44   ` [dpdk-dev] [PATCH v3] " Reshma Pattan
2019-06-24 15:16     ` Thomas Monjalon
2019-06-24 16:48       ` Pattan, Reshma
2019-06-24 16:45     ` [dpdk-dev] [PATCH v4] " Reshma Pattan
2019-06-28  9:53       ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ee4ab6bf-ddf8-cc15-3014-37b4b3818272@intel.com \
    --to=anatoly.burakov@intel.com \
    --cc=david.hunt@intel.com \
    --cc=dev@dpdk.org \
    --cc=liang.j.ma@intel.com \
    --cc=reshma.pattan@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).