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

[Thomas Monjalon <thomas@monjalon.net>]

28/09/2020 15:47, Min Hu (Connor):
> 
> 在 2020/9/28 17:16, Thomas Monjalon 写道:
> > 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?
> > 
> Hi, Thomas,
> you may misunderstand it.
> The goal of this patch is that resolving some errors or warnings when 
> compiling code with RTE_ETHDEV_QUEUE_STAT_CNTRS > 256. The reson is some
> vlaue(uint8_t) will never catch over RTE_ETHDEV_QUEUE_STAT_CNTRS, as
> uint8_t ranges from 0-255. e.g.,
> static void
> nic_stats_display(uint16_t port_id)
> {
> 		uint8_t i;
> 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
> 		...
> 	}
> 	...
> }

What you are describing is not a problem, it is the API:
queue stats mapping is to map few queues (less than 256).

> RTE_ETHDEV_QUEUE_STAT_CNTRS can be increased to more than 256, which is 
> reasonable in principle. Because the number of queue statistics is
> controlled by it. And mapping queue to queue statistics counter is also
> controlled by it.

No, queue statistics ID should be the queue ID (1:1 mapping),
except for rare limited HW.

> So these "*_queue_stats_mapping" API also trigger 
> errors or warning.
> 
> > [...]
> >>> --- 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.
> > 
> >> 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.
> > 
>   First, my modification for "stat_idx" data type is resonable and it
>   does not cause some problems.

Why do you think it is reasonnable?
What is the use case?

>   Both queue id and stat_idx have same
> type, which is also reasoable in usage.
>   Then, "*_queue_stats_mapping" these API is not only invented for ixgbe.
>   other net drivers also implements the API, like enic, igc, octeontx2 etc.

I don't think enic implements it,
and I don't know what is the intent in igc and octeontx2
(maybe just a copy/paste).

> > The problem is probably somewhere else (in testpmd),
> > that's why I am against this patch.

We should start from the beginning: why do you increase this value?

Or we can go directly to the real issue:
testpmd shows queues statistics only if a mapping is defined.
This testpmd behaviour is absolutely wrong and should be fixed.