From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9A044A09EF; Tue, 22 Dec 2020 03:49:32 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7B5C2CA62; Tue, 22 Dec 2020 03:49:30 +0100 (CET) Received: from dish-sg.nttdocomo.co.jp (dish-sg.nttdocomo.co.jp [202.19.227.74]) by dpdk.org (Postfix) with ESMTP id 1FB31CA60 for ; Tue, 22 Dec 2020 03:49:28 +0100 (CET) X-dD-Source: Outbound Received: from zssg-mailmd102.ddreams.local (zssg-mailmd900.ddreams.local [10.160.172.63]) by zssg-mailou104.ddreams.local (Postfix) with ESMTP id 57C3C1200E1; Tue, 22 Dec 2020 11:49:26 +0900 (JST) Received: from t131sg-mailcc111.ddreams.local (t131sg-mailcc111.ddreams.local [100.66.31.221]) by zssg-mailmd102.ddreams.local (dDREAMS) with ESMTP id <0QLP013AUZUEPFC0@dDREAMS>; Tue, 22 Dec 2020 11:49:26 +0900 (JST) Received: from t131sg-mailcc112.ddreams.local (localhost [127.0.0.1]) by t131sg-mailcc111.ddreams.local (unknown) with SMTP id 0BM2nQKS043824; Tue, 22 Dec 2020 11:49:26 +0900 Received: from zssg-mailmf101.ddreams.local (unknown [127.0.0.1]) by zssg-mailmf101.ddreams.local (Postfix) with ESMTP id 54C3A7E603B; Tue, 22 Dec 2020 11:48:43 +0900 (JST) Received: from zssg-mailmf101.ddreams.local (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 50A0A8E6060; Tue, 22 Dec 2020 11:48:43 +0900 (JST) Received: from localhost (unknown [127.0.0.1]) by IMSVA (Postfix) with SMTP id 4CFEE8E605B; Tue, 22 Dec 2020 11:48:43 +0900 (JST) X-IMSS-HAND-OFF-DIRECTIVE: localhost:10026 Received: from zssg-mailmf101.ddreams.local (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 0ED378E6056; Tue, 22 Dec 2020 11:48:43 +0900 (JST) Received: from zssg-mailua103.ddreams.local (unknown [10.160.172.62]) by zssg-mailmf101.ddreams.local (Postfix) with ESMTP; Tue, 22 Dec 2020 11:48:43 +0900 (JST) Received: from [10.87.198.18] (unknown [10.160.183.129]) by zssg-mailua103.ddreams.local (dDREAMS) with ESMTPA id <0QLP0012EZT1B860@dDREAMS>; Tue, 22 Dec 2020 11:48:37 +0900 (JST) Date: Tue, 22 Dec 2020 11:48:36 +0900 From: Hideyuki Yamashita In-reply-to: References: <20201204075109.14694-6-yamashita.hideyuki@ntt-tx.co.jp> Message-id: <20201222114836.CFA3.17218CA3@ntt-tx.co.jp> MIME-version: 1.0 Content-type: text/plain; charset=US-ASCII Content-transfer-encoding: 7bit X-Mailer: Becky! ver. 2.75.01 [ja] X-TM-AS-GCONF: 00 To: "Ananyev, Konstantin" Cc: Thomas Monjalon , "Yigit, Ferruh" , Andrew Rybchenko , Ray Kinsella , Neil Horman , "Burakov, Anatoly" , "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats 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" Hello, Thanks for your comments. Please see my comments inline tagged with [HY]. > Hi, > > > > > This patch modifies to use apistats by librte_ethdev. > > > > Signed-off-by: Hideyuki Yamashita > > --- > > lib/librte_ethdev/meson.build | 6 ++- > > lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++ > > lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++ > > lib/librte_ethdev/rte_ethdev.h | 7 ++++ > > lib/librte_ethdev/version.map | 5 +++ > > 5 files changed, 144 insertions(+), 2 deletions(-) > > create mode 100644 lib/librte_ethdev/rte_apistats.c > > create mode 100644 lib/librte_ethdev/rte_apistats.h > > > > diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build > > index e4b6102..d03e784 100644 > > --- a/lib/librte_ethdev/meson.build > > +++ b/lib/librte_ethdev/meson.build > > @@ -8,7 +8,8 @@ sources = files('ethdev_private.c', > > 'rte_ethdev.c', > > 'rte_flow.c', > > 'rte_mtr.c', > > - 'rte_tm.c') > > + 'rte_tm.c' , > > + 'rte_apistats.c') > > > > headers = files('rte_ethdev.h', > > 'rte_ethdev_driver.h', > > @@ -24,6 +25,7 @@ headers = files('rte_ethdev.h', > > 'rte_mtr.h', > > 'rte_mtr_driver.h', > > 'rte_tm.h', > > - 'rte_tm_driver.h') > > + 'rte_tm_driver.h', > > + 'rte_apistats.h') > > > > deps += ['net', 'kvargs', 'meter', 'telemetry'] > > diff --git a/lib/librte_ethdev/rte_apistats.c b/lib/librte_ethdev/rte_apistats.c > > new file mode 100644 > > index 0000000..c4bde34 > > --- /dev/null > > +++ b/lib/librte_ethdev/rte_apistats.c > > @@ -0,0 +1,64 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2019 NTT TechnoCross Corporation > > + */ > > + > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "rte_apistats.h" > > + > > +/* Macros for printing using RTE_LOG */ > > +#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1 > > + > > +#define MZ_APISTATS "rte_apistats" > > + > > +struct rte_apistats *rte_apicounts; > > + > > +int rte_apistats_init(void) > > +{ > > + int i; > > + const struct rte_memzone *mz = NULL; > > + const unsigned int flags = 0; > > + > > + /** Allocate stats in shared memory fo multi process support */ > > + if (rte_eal_process_type() == RTE_PROC_SECONDARY) { > > + mz = rte_memzone_lookup(MZ_APISTATS); > > + if (mz == NULL) { > > + RTE_LOG(ERR, APISTATS, "Cannot get info structure\n"); > > + return -1; > > + } > > + rte_apicounts = mz->addr; > > + } else { > > + /* RTE_PROC_PRIMARY */ > > + mz = rte_memzone_reserve(MZ_APISTATS, sizeof(*rte_apicounts), > > + rte_socket_id(), flags); > > + if (mz == NULL) { > > + RTE_LOG(ERR, APISTATS, "Cannot reserve memory zone\n"); > > + return -ENOMEM; > > + } > > + rte_apicounts = mz->addr; > > + memset(rte_apicounts, 0, sizeof(*rte_apicounts)); > > + } > > + > > + /* set up array for data */ > > + RTE_LCORE_FOREACH(i) { > > + rte_apicounts->lcoreid_list[i] = 1; > > + RTE_LOG(INFO, APISTATS, "Enable core usage for lcore %u\n", i); > > + } > > + return 0; > > +} > > + > > +int rte_apistats_uninit(void) > > +{ > > + const struct rte_memzone *mz = NULL; > > + /* free up the memzone */ > > + mz = rte_memzone_lookup(MZ_APISTATS); > > + if (mz) > > + rte_memzone_free(mz); > > + return 0; > > +} > > diff --git a/lib/librte_ethdev/rte_apistats.h b/lib/librte_ethdev/rte_apistats.h > > new file mode 100644 > > index 0000000..afea50e > > --- /dev/null > > +++ b/lib/librte_ethdev/rte_apistats.h > > @@ -0,0 +1,64 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2019 NTT TechnoCross Corporation > > + */ > > + > > +#ifndef _RTE_APISTATS_H_ > > +#define _RTE_APISTATS_H_ > > + > > +/** > > + * @file > > + * RTE apistats > > + * > > + * library to provide rte_rx_burst/tx_burst api stats. > > + */ > > + > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +#include > > +#include > > + > > +/** > > + * A structure for rte_rx_burst/tx_burst api statistics. > > + */ > > +struct rte_apistats { > > + int lcoreid_list[RTE_MAX_LCORE]; /**< In use lcoreid list */ > > + /**< Total rte_rx_burst call counts */ > > + uint64_t rx_burst_counts[RTE_MAX_LCORE]; > > + > > + /**< Total rte_tx_burst call counts */ > > + uint64_t tx_burst_counts[RTE_MAX_LCORE]; > > +}; > > + > > +extern struct rte_apistats *rte_apicounts; > > + > > +/** > > + * Initialize rte_rx_burst/tx_burst call count area. > > + * @b EXPERIMENTAL: this API may change without prior notice. > > + * > > + * @return > > + * -1 : On error > > + * -ENOMEM: On error > > + * 0 : On success > > + */ > > +__rte_experimental > > +int rte_apistats_init(void); > > + > > +/** > > + * Clean up and free memory. > > + * @b EXPERIMENTAL: this API may change without prior notice. > > + * > > + * @return > > + * -1: On error > > + * 0: On success > > + */ > > +__rte_experimental > > +int rte_apistats_uninit(void); > > + > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#endif /* _RTE_APISTATS_H_ */ > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h > > index f5f8919..bef9bc6 100644 > > --- a/lib/librte_ethdev/rte_ethdev.h > > +++ b/lib/librte_ethdev/rte_ethdev.h > > @@ -160,6 +160,7 @@ extern "C" { > > > > #include "rte_ethdev_trace_fp.h" > > #include "rte_dev_info.h" > > +#include > > > > extern int rte_eth_dev_logtype; > > > > @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id, > > nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], > > rx_pkts, nb_pkts); > > > > + int lcore_id = rte_lcore_id(); > > + rte_apicounts->rx_burst_counts[lcore_id]++; > > There are few problems with current implementation: > 1. rx_burst() can be called by non-EAL thread (rte_lcore_id() == -1) > In that case it would cause a crash. [HY] Thanks for your info. I think by adding code like following. if(rte_lcore_id() = -1){ return } > 2. Because of the layout of struct rte_apistats it would cause significant > performance degradation (false cache-lines sharing). [HY] I think you are correct. This affects change in core 1 to all other cores even thogh change in core 1 does NOT affect other cores. Root cause is using array like [RTE_MAX_LCORE], correct? I will change it in revised patcheset. +struct rte_apistats { + int lcoreid_list[RTE_MAX_LCORE]; /**< In use lcoreid list */ + /**< Total rte_rx_burst call counts */ + uint64_t rx_burst_counts[RTE_MAX_LCORE]; + + /**< Total rte_tx_burst call counts */ + uint64_t tx_burst_counts[RTE_MAX_LCORE]; +}; > As a generic one: such sort of statistics can be easily collected by the app itself. > Either by just incrementing counters before rx/tx_burst function call directly or > via rx/tx callbacks. > So I don't see any point to push it inside ethdev layer. > Konstantin [HY] You are correct. Application can do it. But if applications want to do it, then every applicaiton needs to do it. The reason why I propose my patchset is to provide such common function (count invocation of rx_burst/tx_burst) so that application only needs to "retrieve the information". I think it is benefitical than all application do similar thing. Your feedback is highly appreciated. Thanks! BR, Hideyuki Yamashita NTT TechnoCross > > #ifdef RTE_ETHDEV_RXTX_CALLBACKS > > struct rte_eth_rxtx_callback *cb; > > > > @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id, > > } > > #endif > > > > + int lcore_id = rte_lcore_id(); > > + rte_apicounts->tx_burst_counts[lcore_id]++; > > #ifdef RTE_ETHDEV_RXTX_CALLBACKS > > struct rte_eth_rxtx_callback *cb; > > > > diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map > > index d3f5410..adea432 100644 > > --- a/lib/librte_ethdev/version.map > > +++ b/lib/librte_ethdev/version.map > > @@ -240,6 +240,11 @@ EXPERIMENTAL { > > rte_flow_get_restore_info; > > rte_flow_tunnel_action_decap_release; > > rte_flow_tunnel_item_release; > > + > > + # added in 21.02 > > + rte_apistats_init; > > + rte_apistats_uninit; > > + rte_apicounts; > > }; > > > > INTERNAL { > > -- > > 2.18.0