Re: [dpdk-dev] [PATCH V2 1/4] ethdev: fix compiling errors for per-queue statistics

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

On 9/25/2020 9:58 AM, Min Hu (Connor) wrote:
> Hi, Ferruh Yigit,
> 
> Your method is very practical. Thanks!
> But we found that testpmd fails to start with "--rx-queue-stats-mapping" 
> and "--tx-queue-stats-mapping". We've found the cause and modified it. 
> These modifications are being tested.
> 
> Currently, the following modifications are related to framework APIs.
> 1)struct 'rte_eth_dcb_tc_queue_mapping'
> 2)fix 'rte_eth_dev_set_rx/tx_queue_stats_mapping' function to resolve
> the problem that DPDK project fails to build when 
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
> 
> According to my current analysis, these modifications for queue stats 
> mapping in testpmd do not involve the modification of framework APIs. 
> Therefore, I think we should give priority to focusing on the current 
> changes to the DPDK framework APIs.
> 
> After all, DPDK-20.11 is approaching. What do you think?
> 

Makes sense, agree to continue with this patchset and queue stats 
mapping related changes can be done later.

> 
> 在 2020/9/23 17:18, Ferruh Yigit 写道:
>> On 9/23/2020 3:31 AM, Min Hu (Connor) wrote:
>>>
>>>
>>> 在 2020/9/5 2:31, Ferruh Yigit 写道:
>>>> On 9/4/2020 12:32 PM, 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 driver and librte_ethdev
>>>>> during compiling dpdk project. But it is possible and permited 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/")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> 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>
>>>>
>>>> The patch mostly looks good and it enables build with
>>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 256.
>>>> Only I put a comment for a testpmd change to enable the "set 
>>>> stat_qmap" command
>>>> for map value > 256.
>>>>
>>>>
>>>> BUT there are many things to fix in the queue stats mapping, since 
>>>> you are
>>>> already on it can you help on a few things on testpmd related to it, 
>>>> if you have
>>>> time for it?
>>>>
>>>> 1) Getting queue stats shouldn't require stats mapping, it should be 
>>>> controlled
>>>> separately. Many PMDs doesn't require/do the stats mapping but they 
>>>> still can
>>>> collect the per queue stats, which can be displayed independent from 
>>>> mapping.
>>>>
>>>> 2) Even you map only one queue, while displaying stats it will display
>>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' queues, and when that number is high 
>>>> it makes hard
>>>> to see the actual interested values.
>>>> If there is no mapping, it should display min(number_of_queues,
>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS).
>>>> If there is mapping it should display queues that mapping done, this 
>>>> may require
>>>> adding a new 'active' field to 'struct queue_stats_mappings'.
>>>>
>>>> 3) Why 'struct queue_stats_mappings' is cache aligned, is it really 
>>>> needed?
>>>>
>>>> 4) The mapping arrays, 'tx_queue_stats_mappings_array' &
>>>> 'rx_queue_stats_mappings_array' are global and their size is based 
>>>> on fixed max
>>>> port and queue size assumptions, can those mapping array be done per 
>>>> port and
>>>> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' size per port?
>>>>
>>> It does seem to be unreasonable. We also try do it, and found that
>>> it is hard to control per queue stats and queue stats mapping, 
>>> separately.
>>> For details, see next V3 patch.
>>
>> I will check v3, but as far as know stats mapping exist because of 
>> limitations of some NICs, that there are N queues but M stats 
>> registers where N > M. So need to map some queues to stat registers, 
>> and there is N:1 relation there, so multiple queue stats can be 
>> represented in single stat register.
>>
>> But for rest of the NICs it should be possible to display the queue 
>> stats independent from the mapping.
>>
>>
>>> We're working on debugging this stats_mapping setting. During this 
>>> process,
>>> we have a problem that starting testpmd with --rx/tx-queue-stats-mapping
>>> parameter fails. log as follows:
>>> [root]$ ./testpmd -l 1,3,4,8,12 -n 4 -w 0000:04:00.0 
>>> --file-prefix=lee --log-level=7
>>>   -- -i --rxq=6 --txq=6 --burst=64 --rxd=2048 --txd=2048 --nb-cores=4
>>>   --rx-queue-stats-mapping=(0,2,2) --tx-queue-stats-mapping=(0,3,3)
>>>
>>> -bash: syntax error near unexpected token `('
>>> [root]$
>>
>> Above seems bash error, using '' can help for it, like 
>> --rx-queue-stats-mapping='(0,2,2)'
>> .