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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/28/2020 4:43 PM, Stephen Hemminger wrote:
> On Mon, 28 Sep 2020 17:24:26 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
>> 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 <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.
>>>>
>>>> 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.
> 
> 
> The original API is a band aid for the limited number of statistics counters
> in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather
> have per-queue statistics and make ixgbe say "not supported"
> 

The current issue is not directly related to '*_queue_stats_mapping' APIs.

Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255.
User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is used 
to define size of the stats counter.
"uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];"

When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors, the 
one in the ethdev is like [1].

This can be fixed two ways,
a) increase the size of 'stat_idx' storage type to u16 in the 
'*_queue_stats_mapping' APIs, this is what this patch does.
b) Fix with a casting in the comparison, without changing the APIs.

I think both are OK, but is (b) more preferable?


[1]
../lib/librte_ethdev/rte_ethdev.c: In function ‘set_queue_stats_mapping’:
../lib/librte_ethdev/rte_ethdev.c:2943:15: warning: comparison is always false 
due to limited range of data type [-Wtype-limits]
  2943 |  if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
       |               ^~