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 <lihuis...@huawei.com> > >>>> > >>>> 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".
