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

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

On 10/10/2020 9:09 AM, Thomas Monjalon wrote:
> 09/10/2020 22:32, Ferruh Yigit:
>> On 10/6/2020 9:33 AM, Olivier Matz wrote:
>>> On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote:
>>>> 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?
>>>
>>> I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is
>>> not modified.
>>>
>>> On the substance, I agree with Thomas that the queue_stats_mapping API
>>> should be replaced by xstats.
>>>
>>
>> This has been discussed in the last technical board meeting, the decision was to
>> use xstats to get queue related statistics [2].
>>
>> But after second look, even if xstats is used to get statistics,
>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' is used, since xstats uses 'rte_eth_stats_get()'
>> to get queue statistics.
>> So for the case device has more than 255 queues, 'RTE_ETHDEV_QUEUE_STAT_CNTRS'
>> still needs to be set > 255 which will cause the build error.
> 
> You're right, when using the old API in xstats implementation,
> we are limited to RTE_ETHDEV_QUEUE_STAT_CNTRS queues.
> 
>> I have an AR to send a deprecation notice to current method to get the queue
>> statistics, and limit the old method to 256 queues. But since xstats is just a
>> wrapped to old method, I am not quite sure how deprecating it will work.
>>
>> @Thomas, @Honnappa, can you give some more insight on the issue?
> 
> It becomes a PMD issue. The PMD implementation of xstats must complete
> the statistics for the queues above RTE_ETHDEV_QUEUE_STAT_CNTRS.
> 
> In order to prepare the removal of the old method smoothly,
> we could add a driver flag which indicates whether the PMD relies
> on a pre-fill of xstats from old stats per queue conversion or not.
> 

I have sent an RFC, can you please check:
https://patches.dpdk.org/patch/80390/


Connor,

Does this proposal make sense?
If you have more than 256 queues to get stats, can you please implement xstats 
for the queue stats?

You don't need to wait for the above RFC accepted, you can implement the xstats, 
but it will have some duplication, if the above RFC accepted you can set the 
'RTE_ETH_DEV_QUEUE_STATS_IN_XSTATS' flag to remove the duplication.

> 
>> [2]
>> https://mails.dpdk.org/archives/dev/2020-October/185299.html
> 
> 
>