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 49AF5A04B1; Sat, 10 Oct 2020 10:09:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9A5B01D70F; Sat, 10 Oct 2020 10:09:37 +0200 (CEST) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by dpdk.org (Postfix) with ESMTP id C0ACD1D709; Sat, 10 Oct 2020 10:09:34 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id AC4E35C00F4; Sat, 10 Oct 2020 04:09:30 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Sat, 10 Oct 2020 04:09:30 -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= KYLPsuBKMnMvxArwz41d1VkuvAuGjYB1T17DqEso9lE=; b=oWpIzBYqmpr9gfQ0 3+HbmgXcPEsd/gU7Bz1dJ7/tCHk9pDQ39itDXGnPYXTnivCGh1JsyOU4neOnHDMi nSt1hisbcsWlIp4xnSxkAviIRnCcE+56b+PEL4il7mMnYFO/373Rw/PDKXTtFJxZ ze/o20n6kwcdlypKAXSDwa5GBhL2q+EJQadAJKYyNd8SfqmVIG6aY7wE5lL6PmfE lWhrLgrj8dCFDG3j+WOWR5P2Szn5QdI9gCwt6u3NphTKAYJmEde5t5CpSENF6jLp aM/wYWadE5gxO+AvI3vEMyH869yOozmCeTdXXhijrRUMP7STNd7lg0e+F9KBwoKy ySANYQ== 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=fm1; bh=KYLPsuBKMnMvxArwz41d1VkuvAuGjYB1T17DqEso9 lE=; b=mTIVf3i3d0UoMTupvUXnoWPDD4to9kQntZWcOwEhmqAO6w2Y9uc58dz8P xD16pr36D0fXrW/OHNUWYxYdixhjFJIoWZjulU5Ayl1C03o6i4zVzV+ogpx59Y/G vTAPU1YnNqmh/P2zeT62w5M/P2dDbY7JbrUJ2oqCBHWZAUBHhC5zhiZEsSXOoi/c twTRgKDfkh18VyLLuWT0Jx6wXw/96Xe/qseYCX8rx0n17X7Cn/gfziMDOijf2czM 1IoL55hKnEbQndmhTnH5+LcJq+8Ia3mqqOWX+JwtiGt3EtCeQ7ie60ANWtjdT3B3 uA6HsFbkJN+eYa2LsE/RITkg9P/RQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrhedvgdduvdelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepffdvffejueetleefieeludduuefgteejleevfeekjeefieegheet ffdvkeefgedunecuffhomhgrihhnpeguphgukhdrohhrghenucfkphepjeejrddufeegrd dvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhf rhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth 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 BA5D93280063; Sat, 10 Oct 2020 04:09:28 -0400 (EDT) From: Thomas Monjalon To: "Min Hu (Connor)" , Honnappa Nagarahalli , Ferruh Yigit Cc: Olivier Matz , Stephen Hemminger , "techboard@dpdk.org" , bruce.richardson@intel.com, "jerinj@marvell.com" , Ray Kinsella , dev@dpdk.org Date: Sat, 10 Oct 2020 10:09:24 +0200 Message-ID: <8303247.AAZLxQO0S4@thomas> In-Reply-To: References: <1598845317-55956-1-git-send-email-humin29@huawei.com> <20201006083325.GF21395@platinum> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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" 09/10/2020 22:32, Ferruh Yigit: > On 10/6/2020 9:33 AM, Olivier Matz wrote: > > On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote: > >> On 9/28/2020 4:43 PM, Stephen Hemminger wrote: > >>> On Mon, 28 Sep 2020 17:24:26 +0200 > >>> Thomas Monjalon wrote: > >>>> 28/09/2020 15:53, Ferruh Yigit: > >>>>> 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 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 previous > >>>>>>> release(s). > >>>>>>> > >>>>>>> It is mainly a fix to the port_id storage type, which we have updated 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. > >>>>> > >>>>> Right, it is not related to the port id, it is fixing the storage type for index > >>>>> used to map the queue stats. > >>>>>>> Since the ethdev library already heavily breaks the ABI this release, 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. > >>>>> > >>>>> This patch is not to fix queue stats mapping, I agree there are problems related > >>>>> to it, already shared as comment to this set. > >>>>> > >>>>> But 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. > >>>> > >>>> It is not said this API is supposed to manage more than 256 queues mapping. > >>>> In general we should not need this API. > >>>> I think it is solving the wrong problem. > >>> > >>> > >>> The original API is a band aid for the limited number of statistics counters > >>> in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather > >>> have per-queue statistics and make ixgbe say "not supported" > >>> > >> > >> The current issue is not directly related to '*_queue_stats_mapping' APIs. > >> > >> Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255. > >> User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is > >> used to define size of the stats counter. > >> "uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];" > >> > >> When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors, > >> the one in the ethdev is like [1]. > >> > >> This can be fixed two ways, > >> a) increase the size of 'stat_idx' storage type to u16 in the > >> '*_queue_stats_mapping' APIs, this is what this patch does. > >> b) Fix with a casting in the comparison, without changing the APIs. > >> > >> I think both are OK, but is (b) more preferable? > > > > I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is > > not modified. > > > > On the substance, I agree with Thomas that the queue_stats_mapping API > > should be replaced by xstats. > > > > This has been discussed in the last technical board meeting, the decision was to > use xstats to get queue related statistics [2]. > > But after second look, even if xstats is used to get statistics, > 'RTE_ETHDEV_QUEUE_STAT_CNTRS' is used, since xstats uses 'rte_eth_stats_get()' > to get queue statistics. > So for the case device has more than 255 queues, 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > still needs to be set > 255 which will cause the build error. You're right, when using the old API in xstats implementation, we are limited to RTE_ETHDEV_QUEUE_STAT_CNTRS queues. > I have an AR to send a deprecation notice to current method to get the queue > statistics, and limit the old method to 256 queues. But since xstats is just a > wrapped to old method, I am not quite sure how deprecating it will work. > > @Thomas, @Honnappa, can you give some more insight on the issue? It becomes a PMD issue. The PMD implementation of xstats must complete the statistics for the queues above RTE_ETHDEV_QUEUE_STAT_CNTRS. In order to prepare the removal of the old method smoothly, we could add a driver flag which indicates whether the PMD relies on a pre-fill of xstats from old stats per queue conversion or not. > [2] > https://mails.dpdk.org/archives/dev/2020-October/185299.html