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 CA110A04B1; Tue, 29 Sep 2020 15:46:43 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2B0A61D523; Tue, 29 Sep 2020 15:46:41 +0200 (CEST) Received: from huawei.com (szxga05-in.huawei.com [45.249.212.191]) by dpdk.org (Postfix) with ESMTP id 2DC891C002; Tue, 29 Sep 2020 15:46:38 +0200 (CEST) Received: from DGGEMS410-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 515AD5B154F7AD2BABB5; Tue, 29 Sep 2020 21:46:36 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS410-HUB.china.huawei.com (10.3.19.210) with Microsoft SMTP Server id 14.3.487.0; Tue, 29 Sep 2020 21:46:34 +0800 To: Thomas Monjalon , Ferruh Yigit CC: "techboard@dpdk.org" , , , "jerinj@marvell.com" , "Ray Kinsella" , References: <1598845317-55956-1-git-send-email-humin29@huawei.com> <03eaf7f8-0a36-1173-b520-5f7ffcc9b87c@huawei.com> <4271893.Xu3jSLBPRA@thomas> From: "Min Hu (Connor)" Message-ID: Date: Tue, 29 Sep 2020 21:46:35 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <4271893.Xu3jSLBPRA@thomas> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected 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" Hi, Thomas, I think what you suggest is resonable and helpful. But I should have time to fix it about this patch. So, this patch will be sent to community in future. By the way, the other patch is acked by you. Acked-by: Thomas Monjalon This patch is : "[V5,2/2] ethdev: change data type in TC rxq and TC txq" https://patches.dpdk.org/patch/78888/ Because we will have National Day Holiday from 10/1 to 10/8, I will have no time to handle the patch. So, I wish that patch can be merged into V20.11 soon. Thanks very much. 在 2020/9/29 17:33, Thomas Monjalon 写道: > 29/09/2020 06:49, Min Hu (Connor): >> >> 在 2020/9/28 21: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. >> >> Yes, Ferruh is right. >> >> Hi, Thomas, >> Let me desribe the background again. >> >> 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: >> rte_eth_dev_set_tx_queue_stats_mapping(or >> rte_eth_dev_set_rx_queue_stats_mapping), this is: >> >> rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,uint16_t >> tx_queue_id, uint8_t stat_idx): >> set_queue_stats_mapping: >> ... >> if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS) >> return -EINVAL; >> .... >> >> As above, stat_idx is uint8_t, range from 0-255, but >> RTE_ETHDEV_QUEUE_STAT_CNTRS is 256 (ever larger). >> So compiling will catch warnings or errors, and failed. >> >> 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 >> that is come up by Ferruh. In addition, we also modify other >> unreasonable something about it and we are testing. >> But it will only modify testpmd, not related to API. >> >> 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: > - it makes basic stats huge and slow to retrieve > - 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". > > > . >