From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 18AD1A052A; Tue, 26 Jan 2021 19:27:00 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 97443140F3A; Tue, 26 Jan 2021 19:26:59 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id 674A5140F33 for ; Tue, 26 Jan 2021 19:26:57 +0100 (CET) IronPort-SDR: dHX+yzXuQqRva6z97cZPL+QknT2wFMFfwq2glHiGcJsaapeIgsxl2RiB9FcDJRS6uDTU39BWB8 hHN6B+Zm8YLQ== X-IronPort-AV: E=McAfee;i="6000,8403,9876"; a="198740353" X-IronPort-AV: E=Sophos;i="5.79,377,1602572400"; d="scan'208";a="198740353" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 10:26:56 -0800 IronPort-SDR: J08QUpjI7bvy0TlCVUW1Kp3HTUfOuBLIG65LgCFe/vh3DqCFJHmx21OOjwW6Pnem4eFew54koN Q4i5JN0a6uPw== X-IronPort-AV: E=Sophos;i="5.79,377,1602572400"; d="scan'208";a="362082160" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.227.53]) ([10.213.227.53]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 10:26:55 -0800 To: lironh@marvell.com, jerinj@marvell.com Cc: dev@dpdk.org, Yuri Chipchev References: <20201202101212.4717-1-lironh@marvell.com> <20210122191925.24308-1-lironh@marvell.com> <20210122191925.24308-9-lironh@marvell.com> From: Ferruh Yigit Message-ID: Date: Tue, 26 Jan 2021 18:26:51 +0000 MIME-Version: 1.0 In-Reply-To: <20210122191925.24308-9-lironh@marvell.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 08/37] net/mvpp2: extend xstats support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 1/22/2021 7:18 PM, lironh@marvell.com wrote: > From: Yuri Chipchev > > Add xstats_by_id callbacks > > Signed-off-by: Yuri Chipchev > Reviewed-by: Liron Himi > --- > drivers/net/mvpp2/mrvl_ethdev.c | 98 ++++++++++++++++++++++++++++++++- > 1 file changed, 95 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c > index e4ec343d5..ba1882266 100644 > --- a/drivers/net/mvpp2/mrvl_ethdev.c > +++ b/drivers/net/mvpp2/mrvl_ethdev.c > @@ -173,6 +173,8 @@ static struct { > MRVL_XSTATS_TBL_ENTRY(tx_errors) > }; > > +#define MRVL_NUM_XSTATS RTE_DIM(mrvl_xstats_tbl) > + > static inline void > mrvl_fill_shadowq(struct mrvl_shadow_txq *sq, struct rte_mbuf *buf) > { > @@ -1376,7 +1378,7 @@ mrvl_xstats_get(struct rte_eth_dev *dev, > return 0; > > pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0); > - for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) { > + for (i = 0; i < n && i < MRVL_NUM_XSTATS; i++) { > uint64_t val; > > if (mrvl_xstats_tbl[i].size == sizeof(uint32_t)) > @@ -1430,9 +1432,9 @@ mrvl_xstats_get_names(struct rte_eth_dev *dev __rte_unused, > unsigned int i; > > if (!xstats_names) > - return RTE_DIM(mrvl_xstats_tbl); > + return MRVL_NUM_XSTATS; > > - for (i = 0; i < size && i < RTE_DIM(mrvl_xstats_tbl); i++) > + for (i = 0; i < size && i < MRVL_NUM_XSTATS; i++) > strlcpy(xstats_names[i].name, mrvl_xstats_tbl[i].name, > RTE_ETH_XSTATS_NAME_SIZE); > > @@ -2015,6 +2017,94 @@ mrvl_eth_filter_ctrl(struct rte_eth_dev *dev __rte_unused, > } > } > > +/** > + * DPDK callback to get xstats by id. > + * > + * @param dev > + * Pointer to the device structure. > + * @param ids > + * Pointer to the ids table. > + * @param values > + * Pointer to the values table. > + * @param n > + * Values table size. > + * @returns > + * Number of read values, negative value otherwise. > + */ > +static int > +mrvl_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids, > + uint64_t *values, unsigned int n) > +{ > + struct rte_eth_xstat xstats[MRVL_NUM_XSTATS]; > + uint16_t i; > + > + if (n < MRVL_NUM_XSTATS && ids == NULL) > + return MRVL_NUM_XSTATS; > + > + if (n > MRVL_NUM_XSTATS) > + return -EINVAL; Why 'n > MRVL_NUM_XSTATS' is an error? I am not aware of this kind of restriction, can you please point if I am missing it. > + > + if (values == NULL) > + return -ENOMEM; > + > + mrvl_xstats_get(dev, xstats, n); > + What if 'n' is a small number, like in a case only single id is requested, so n==1, will above work? Overall there is ethdev level implementation of "_by_id" APIs, if driver has '.xstats_get' & '.xstats_get_names' implemented. Unless driver has more performant way to get these ids, I suggest using the ethdev layer ones, and drop these. > + for (i = 0; i < MRVL_NUM_XSTATS; i++) { > + if (ids[i] >= MRVL_NUM_XSTATS) { > + MRVL_LOG(ERR, "id value is not valid\n"); > + return -EINVAL; > + } > + values[i] = xstats[ids[i]].value; > + } Why this loop goes up to 'MRVL_NUM_XSTATS', does it assumes "n == MRVL_NUM_XSTATS", if so this assumption can be wrong. > + > + return n; > +} > + > +/** > + * DPDK callback to get xstats names by ids. > + * > + * @param dev > + * Pointer to the device structure. > + * @param xstats_names > + * Pointer to table with xstats names. > + * @param ids > + * Pointer to table with ids. > + * @param size > + * Xstats names table size. > + * @returns > + * Number of names read, negative value otherwise. > + */ > +static int > +mrvl_xstats_get_names_by_id(struct rte_eth_dev *dev, > + struct rte_eth_xstat_name *xstats_names, > + const uint64_t *ids, unsigned int size) > +{ > + struct rte_eth_xstat_name names[MRVL_NUM_XSTATS]; > + uint16_t i; > + > + if (size < MRVL_NUM_XSTATS && ids == NULL) > + return MRVL_NUM_XSTATS; > + > + if (size > MRVL_NUM_XSTATS) > + return -EINVAL; > + > + if (xstats_names == NULL) > + return -ENOMEM; > + > + mrvl_xstats_get_names(dev, names, size); > + > + for (i = 0; i < MRVL_NUM_XSTATS; i++) { > + if (ids[i] >= MRVL_NUM_XSTATS) { > + MRVL_LOG(ERR, "id value is not valid"); > + return -EINVAL; > + } > + strncpy(xstats_names[i].name, names[ids[i]].name, > + sizeof(xstats_names[i].name)); > + } > + > + return size; > +} > + > /** > * DPDK callback to get rte_mtr callbacks. > * > @@ -2090,6 +2180,8 @@ static const struct eth_dev_ops mrvl_ops = { > .rss_hash_update = mrvl_rss_hash_update, > .rss_hash_conf_get = mrvl_rss_hash_conf_get, > .filter_ctrl = mrvl_eth_filter_ctrl, > + .xstats_get_by_id = mrvl_xstats_get_by_id, > + .xstats_get_names_by_id = mrvl_xstats_get_names_by_id, > .mtr_ops_get = mrvl_mtr_ops_get, > .tm_ops_get = mrvl_tm_ops_get, > }; >