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

[Thomas Monjalon <thomas@monjalon.net>]

04/09/2020 20: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?

Yes there are a lot of things to review in this area.
For reference, the slides I presented last year:
https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/WhichStandard.pdf

I think we should fix the xstats name "rx_q%u%s" to "rx_q%u_%s".