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 689E2A04C0; Tue, 29 Sep 2020 11:34:00 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B8C7C1D625; Tue, 29 Sep 2020 11:33:58 +0200 (CEST) Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by dpdk.org (Postfix) with ESMTP id 06CA81D60C; Tue, 29 Sep 2020 11:33:56 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 177DB5C0185; Tue, 29 Sep 2020 05:33:52 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Tue, 29 Sep 2020 05:33:52 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm2; bh= euPuAYYvmaRNQuY7iTI+BGf5wguIfdft/+yvzDahcpQ=; b=WK2u04oJjWxFJSxQ 9cDjr4Xm6GBfOSGXaCzy1fbJpCzUWwqTu3dP+h7W49iwTV2QN8Y/87VyZrKvvHLI SwC/BJXaWory34WfM87Y8s5cEUlsVN+MndpENGfw5mKpExcisSmn7fxGNrOZi5fO MVUC9Yy+B1/UeGioATK41mYjbIBOoF3a3P995ZEciUYqvPzFAc5hP1SRl3RCntC1 Kt/jkvotEp9ymVvT8pfPazdjJCr0dRkhYkl6n+RvSLW9Dx4YbllRT/whUYWHLxsd cvNYc58qXD9jrV0gmu+GNW/3F/aXSgN5Ab+0zP511JrEfV5VOv7G76J0xnBPzznO o4KUeA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=euPuAYYvmaRNQuY7iTI+BGf5wguIfdft/+yvzDahc pQ=; b=IAVtXfiP5l7S2Z2CTuHQVQUJHKnfIQVc9I12m2o66jYEfdHwFoUGx7X2y /VOJ7u9FnFA6HlmFwOgakqIXMjPKHykg2EsLiU53tUeYBzi2moYuaz4ARm8qU90C hxS4e/v98qB41lXddsxsFeYumEsrOrx2+NuOOs8W7FLCwFqvxX7c59euVm30z0rR hsXhl8KrA4FjqI0HLtU4+ezzfRtzXxbWs9CGs0vuJvJQwEcDVkgEDmHM4dgMXG1D L3/OYIiJSE70/BVRoRIfOF28+H7IN5CAqFqLP6VrKi6BStRgNYjqxQYuFbAd3Ixl cZDN6X9IdF/9gpSphR+t4W83T334g== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrvdekgddujecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthhqredttddtjeenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpedvkeetveeihfegfedtfeejueekkeekueevgfejuedviedvvdevuefg teevtdefveenucffohhmrghinhepughpughkrdhorhhgnecukfhppeejjedrudefgedrvd dtfedrudekgeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 564FE3064683; Tue, 29 Sep 2020 05:33:50 -0400 (EDT) From: Thomas Monjalon To: Ferruh Yigit , "Min Hu (Connor)" Cc: "techboard@dpdk.org" , stephen@networkplumber.org, bruce.richardson@intel.com, "jerinj@marvell.com" , Ray Kinsella , dev@dpdk.org Date: Tue, 29 Sep 2020 11:33:48 +0200 Message-ID: <4271893.Xu3jSLBPRA@thomas> In-Reply-To: <03eaf7f8-0a36-1173-b520-5f7ffcc9b87c@huawei.com> References: <1598845317-55956-1-git-send-email-humin29@huawei.com> <03eaf7f8-0a36-1173-b520-5f7ffcc9b87c@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [dpdk-techboard] [PATCH V5 1/2] dpdk: resolve compiling errors for per-queue stats 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" 29/09/2020 06:49, Min Hu (Connor): >=20 > =E5=9C=A8 2020/9/28 21:53, Ferruh Yigit =E5=86=99=E9=81=93: > > On 9/28/2020 10:16 AM, Thomas Monjalon wrote: > >> 28/09/2020 10:59, Ferruh Yigit: > >>> On 9/27/2020 4:16 AM, Min Hu (Connor) wrote: > >>>> From: Huisong Li > >>>> > >>>> Currently, only statistics of rx/tx queues with queue_id less than > >>>> RTE_ETHDEV_QUEUE_STAT_CNTRS can be displayed. If there is a certain > >>>> application scenario that it needs to use 256 or more than 256 queues > >>>> and display all statistics of rx/tx queue. At this moment, we have to > >>>> change the macro to be equaled to the queue number. > >>>> > >>>> However, modifying the macro to be greater than 256 will trigger > >>>> many errors and warnings from test-pmd, PMD drivers and librte_ethdev > >>>> during compiling dpdk project. But it is possible and permitted that > >>>> rx/tx queue number is greater than 256 and all statistics of rx/tx > >>>> queue need to be displayed. In addition, the data type of rx/tx queue > >>>> number in rte_eth_dev_configure API is 'uint16_t'. So It is=20 > >>>> unreasonable > >>>> to use the 'uint8_t' type for variables that control which per-queue > >>>> statistics can be displayed. > >> > >> The explanation is too much complex and misleading. > >> You mean you cannot increase RTE_ETHDEV_QUEUE_STAT_CNTRS > >> above 256 because it is an 8-bit type? > >> > >> [...] > >>>> --- a/lib/librte_ethdev/rte_ethdev.h > >>>> +++ b/lib/librte_ethdev/rte_ethdev.h > >>>> int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id, > >>>> - uint16_t tx_queue_id, uint8_t stat_idx); > >>>> + uint16_t tx_queue_id, uint16_t stat_idx); > >> [...] > >>>> int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id, > >>>> uint16_t rx_queue_id, > >>>> - uint8_t stat_idx); > >>>> + uint16_t stat_idx); > >> [...] > >>> cc'ed tech-board, > >>> > >>> The patch breaks the ethdev ABI without a deprecation notice from=20 > >>> previous > >>> release(s). > >>> > >>> It is mainly a fix to the port_id storage type, which we have updated= =20 > >>> from > >>> uint8_t to uint16_t in past but some seems remained for > >>> 'rte_eth_dev_set_tx_queue_stats_mapping()' & > >>> 'rte_eth_dev_set_rx_queue_stats_mapping()' APIs. > >> > >> No, it is not related to the port id, but the number of limited stats. > >> > >=20 > > Right, it is not related to the port id, it is fixing the storage type= =20 > > for index used to map the queue stats. > >=20 > >>> Since the ethdev library already heavily breaks the ABI this release,= =20 > >>> I am for > >>> getting this fix, instead of waiting the fix for one more year. > >> > >> If stats can be managed for more than 256 queues, I think it means > >> it is not limited. In this case, we probably don't need the API > >> *_queue_stats_mapping which was invented for a limitation of ixgbe. > >> > >> The problem is probably somewhere else (in testpmd), > >> that's why I am against this patch. > >> > >=20 > > This patch is not to fix queue stats mapping, I agree there are problem= s=20 > > related to it, already shared as comment to this set. > >=20 > > But this patch is to fix the build errors when=20 > > 'RTE_ETHDEV_QUEUE_STAT_CNTRS' needs to set more than 255. Where the=20 > > build errors seems around the stats_mapping APIs. >=20 > Yes, Ferruh is right. >=20 > Hi, Thomas, > Let me desribe the background again. >=20 > There exists a certain application scenario: it needs to use 256 > or more than 256 queues and display all statistics of rx/tx queues. > In this scenario, RTE_ETHDEV_QUEUE_STAT_CNTRS should be modifed to > 256 or more for the NICs supporting per-queue statistics. No, RTE_ETHDEV_QUEUE_STAT_CNTRS does not have to be changed. The queue statistics should be retrieved with xstats, and abandoned in rte_eth_stats. But yes nothing is currently forbidding increasing RTE_ETHDEV_QUEUE_STAT_CNTRS, except build issues. > While if we did this, compiling codes will cause warnings or errors. > WHY? one reson occures in this API:=20 > rte_eth_dev_set_tx_queue_stats_mapping(or=20 > rte_eth_dev_set_rx_queue_stats_mapping), this is: >=20 > rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,uint16_t=20 > tx_queue_id, uint8_t stat_idx): > set_queue_stats_mapping: > ... > if (stat_idx >=3D RTE_ETHDEV_QUEUE_STAT_CNTRS) > return -EINVAL; > .... >=20 > As above, stat_idx is uint8_t, range from 0-255, but=20 > RTE_ETHDEV_QUEUE_STAT_CNTRS is 256 (ever larger). > So compiling will catch warnings or errors, and failed. >=20 > So, as Ferruh said, > this patch is to fix the build errors when > 'RTE_ETHDEV_QUEUE_STAT_CNTRS' needs to set more than 255. Where the > build errors seems around the stats_mapping APIs. Really I would prefer deprecating RTE_ETHDEV_QUEUE_STAT_CNTRS, instead of increasing size of the mapping API. > Really, seting queue stats mapping in testpmd also has some problems=20 > that is come up by Ferruh. In addition, we also modify other=20 > unreasonable something about it and we are testing. > But it will only modify testpmd, not related to API. >=20 > Hope for your reply, thanks. I understand the issue, you're right raising it, but I think the usage of these APIs is wrong. I would prefer we work on xstats instead of allowing more ugly stuff with legacy stats per queues. 2 problems with increasing RTE_ETHDEV_QUEUE_STAT_CNTRS: =2D it makes basic stats huge and slow to retrieve =2D it needs to recompile DPDK I proposed a scheme for stats per queue in this presentation: https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/WhichStandard.pdf If we were at breaking something, we should change the xstats scheme from "rx_q%u%s" to "rx_q%u_%s".