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 <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.
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.
