Re: [dpdk-dev] [PATCH V5 1/2] dpdk: resolve compiling errors for per-queue stats

["Kinsella, Ray" <mdr@ashroe.eu>]

On 28/09/2020 09:59, Ferruh Yigit wrote:
> On 9/27/2020 4:16 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@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.
>>
>> Fixes: ed30d9b691b2 ("app/testpmd: add stats per queue")
>> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
>> Fixes: abf7275bbaa2 ("ixgbe: move to drivers/net/")
>> Fixes: e6defdfddc3b ("net/igc: enable statistics")
>> Fixes: 2265e4b4e84b ("net/octeontx2: add basic stats operation")
>> Fixes: 6c3169a3dc04 ("virtio: move to drivers/net/")
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>> V4 -> V5:
>> add release notes updated.
>>
>> ---
>> v3->v4:
>> add a change in cmd_setqmap_mapvalue.
>>
>> ---
>> v2->v3:
>> change 'uint8_t i' to 'uint16_t i' in nic_stats_display function.
>>
>> ---
>>   app/proc-info/main.c                   | 2 +-
>>   app/test-pmd/cmdline.c                 | 4 ++--
>>   app/test-pmd/config.c                  | 4 ++--
>>   app/test-pmd/testpmd.c                 | 2 +-
>>   app/test-pmd/testpmd.h                 | 5 +++--
>>   doc/guides/rel_notes/release_20_11.rst | 5 +++++
>>   drivers/net/igc/igc_ethdev.c           | 4 ++--
>>   drivers/net/ixgbe/ixgbe_ethdev.c       | 4 ++--
>>   drivers/net/memif/rte_eth_memif.c      | 2 +-
>>   drivers/net/octeontx2/otx2_ethdev.h    | 2 +-
>>   drivers/net/octeontx2/otx2_stats.c     | 2 +-
>>   drivers/net/virtio/virtio_ethdev.c     | 4 ++--
>>   lib/librte_ethdev/rte_ethdev.c         | 6 +++---
>>   lib/librte_ethdev/rte_ethdev.h         | 4 ++--
>>   lib/librte_ethdev/rte_ethdev_driver.h  | 2 +-
>>   15 files changed, 29 insertions(+), 23 deletions(-)
>>
[SNIP]
> 
> 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.
> 
> 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.
> 
> Can you please review the patch, is there any objectio to proceed with it?

After reading the rest of the thread, I understand that Thomas has suggested depreciating this entire API and using xstats instead.
My 2c is that if changing this value requires an ABI breakage and rebuild, its probably the wrong API.

Ray K