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

[Thomas Monjalon <thomas@monjalon.net>]

29/09/2020 06:49, Min Hu (Connor):
> 
> 在 2020/9/28 21:53, Ferruh Yigit 写道:
> > On 9/28/2020 10:16 AM, Thomas Monjalon wrote:
> >> 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?
> >>
> >> [...]
> >>>> --- 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.
> >>
> > 
> > Right, it is not related to the port id, it is fixing the storage type 
> > for index used to map the queue 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.
> >>
> >> The problem is probably somewhere else (in testpmd),
> >> that's why I am against this patch.
> >>
> > 
> > This patch is not to fix queue stats mapping, I agree there are problems 
> > related to it, already shared as comment to this set.
> > 
> > But this patch is to fix the build errors when 
> > 'RTE_ETHDEV_QUEUE_STAT_CNTRS' needs to set more than 255. Where the 
> > build errors seems around the stats_mapping APIs.
> 
> Yes, Ferruh  is right.
> 
> Hi, Thomas,
> Let me desribe the background again.
> 
> There exists a certain application scenario: it needs to use 256
> or more than 256 queues and display all statistics of rx/tx queues.
> In this scenario, RTE_ETHDEV_QUEUE_STAT_CNTRS should be modifed to
> 256 or more for the NICs supporting per-queue statistics.

No, RTE_ETHDEV_QUEUE_STAT_CNTRS does not have to be changed.
The queue statistics should be retrieved with xstats,
and abandoned in rte_eth_stats.

But yes nothing is currently forbidding increasing
RTE_ETHDEV_QUEUE_STAT_CNTRS, except build issues.

> While if we did this, compiling codes will cause warnings or errors.
> WHY? one reson occures in this API: 
> rte_eth_dev_set_tx_queue_stats_mapping(or 
> rte_eth_dev_set_rx_queue_stats_mapping), this is:
> 
> rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,uint16_t 
> tx_queue_id, uint8_t stat_idx):
> 	set_queue_stats_mapping:
> 			...
> 			if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS)
> 			return -EINVAL;
> 			....
> 
> As above, stat_idx  is uint8_t, range from 0-255, but 
> RTE_ETHDEV_QUEUE_STAT_CNTRS is 256 (ever larger).
> So compiling will catch warnings or errors, and failed.
> 
> So, as Ferruh said,
> this patch is to fix the build errors when
> 'RTE_ETHDEV_QUEUE_STAT_CNTRS' needs to set more than 255. Where the
> build errors seems around the stats_mapping APIs.

Really I would prefer deprecating RTE_ETHDEV_QUEUE_STAT_CNTRS,
instead of increasing size of the mapping API.

> Really, seting queue stats mapping in testpmd also has some problems 
> that is come up by Ferruh.  In addition, we also modify other 
> unreasonable something about it and we are testing.
> But it will only modify testpmd, not related to API.
> 
> Hope for your reply, thanks.

I understand the issue, you're right raising it,
but I think the usage of these APIs is wrong.
I would prefer we work on xstats instead of allowing
more ugly stuff with legacy stats per queues.

2 problems with increasing RTE_ETHDEV_QUEUE_STAT_CNTRS:
- it makes basic stats huge and slow to retrieve
- it needs to recompile DPDK

I proposed a scheme for stats per queue in this presentation:
https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/WhichStandard.pdf

If we were at breaking something, we should change the xstats scheme
from "rx_q%u%s" to "rx_q%u_%s".