From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id F2AE8A05D3 for ; Mon, 20 May 2019 15:17:34 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EEBBB569B; Mon, 20 May 2019 15:17:33 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 445E0568A for ; Mon, 20 May 2019 15:17:31 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 May 2019 06:17:30 -0700 X-ExtLoop1: 1 Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.70]) ([10.237.220.70]) by orsmga008.jf.intel.com with ESMTP; 20 May 2019 06:17:28 -0700 To: Reshma Pattan , dev@dpdk.org Cc: david.hunt@intel.com, liang.j.ma@intel.com References: <20190517181712.12027-1-reshma.pattan@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Mon, 20 May 2019 14:17:28 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190517181712.12027-1-reshma.pattan@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1] examples/l3fwd-power: add telemetry mode support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > + 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" > > 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; > + 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); > + } > /* 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