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

[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 <lihuisong at 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.