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

["Min Hu (Connor)" <humin29@huawei.com>]

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?


在 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)'
> .