Re: [dpdk-dev] [RFC V2 1/2] app/testpmd: fix queue stats mapping configuration

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

On 10/20/2020 9:26 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> Currently, the queue stats mapping has the following problems:
> 1) Many PMD drivers don't support queue stats mapping. But there is no
> failure message after executing the command "set stat_qmap rx 0 2 2".
> 2) Once queue mapping is set, unrelated and unmapped queues are also
> displayed.
> 3) There is no need to keep cache line alignment for
> 'struct queue_stats_mappings'.
> 4) The mapping arrays, 'tx_queue_stats_mappings_array' &
> 'rx_queue_stats_mappings_array' are global and their sizes are based on
> fixed max port and queue size assumptions.
> 5) The configuration result does not take effect or can not be queried
> in real time.
> 
> Therefore, we have made the following adjustments:
> 1) If PMD supports queue stats mapping, configure to driver in real time
> after executing the command "set stat_qmap rx/tx ...". If not,
> the command can not be accepted.
> 2) Only display queues that mapping done by adding a new 'active' field
>   in queue_stats_mappings struct.
> 3) Remove cache alignment for 'struct queue_stats_mappings'.
> 4) Add a new port_stats_mappings struct in rte_port.
> The struct contains number of rx/txq stats mapping, rx/tx
> queue_stats_mapping_enabled flag, and rx/tx queue_stats_mapping array.
> Size of queue_stats_mapping_array is set to "RTE_ETHDEV_QUEUE_STAT_CNTRS"
>   to ensure that the same number of queues can be set for each port.
> 

Hi Connor,

I think above adjustment are good, but after the decision to use xstats for the 
queue stats, what do you think about more simplification,

1)
What testpmd does is, records the queue stats mapping commands and registers 
them later on port start & forwarding start.
What happens if recording and registering completely removed?
When "set stat_qmap .." issued, it just call the ethdev APIs to do the mapping 
in device.
This lets us removing record structures, "struct port_stats_mappings p_stats_map"
Also can remove 'map_port_queue_stats_mapping_registers()' and its sub functions.

2)
Also lets remove "tx-queue-stats-mapping" & "rx-queue-stats-mapping" parameters, 
which enables removing 'parse_queue_stats_mapping_config()' function too

3)
Another problem is to display the queue stats, in 'fwd_stats_display()' & 
'nic_stats_display()', there is a check if the queue stats mapping enable or not 
('rx_queue_stats_mapping_enabled' & 'tx_queue_stats_mapping_enabled'),
I think displaying queue stats and queue stat mapping should be separate, why 
not drop checks for queue stats mapping and display queue stats for 'nb_rxq' & 
'nb_txq' queues?

Does above make sense?


Majority of the drivers doesn't require queue stat mapping to get the queue 
stats, lets don't pollute main usage with this requirement.


> Fixes: 4dccdc789bf4b ("app/testpmd: simplify handling of stats mappings error")
> Fixes: 013af9b6b64f6 ("app/testpmd: various updates")
> Fixes: ed30d9b691b21 ("app/testpmd: add stats per queue")
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>

<...>