09/10/2020 22:32, Ferruh Yigit: > On 10/6/2020 9:33 AM, Olivier Matz wrote: > > On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote: > >> On 9/28/2020 4:43 PM, Stephen Hemminger wrote: > >>> On Mon, 28 Sep 2020 17:24:26 +0200 > >>> Thomas Monjalon <tho...@monjalon.net> wrote: > >>>> 28/09/2020 15: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 <lihuis...@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. > >>>> > >>>> It is not said this API is supposed to manage more than 256 queues > >>>> mapping. > >>>> In general we should not need this API. > >>>> I think it is solving the wrong problem. > >>> > >>> > >>> The original API is a band aid for the limited number of statistics > >>> counters > >>> in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would > >>> rather > >>> have per-queue statistics and make ixgbe say "not supported" > >>> > >> > >> The current issue is not directly related to '*_queue_stats_mapping' APIs. > >> > >> Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255. > >> User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is > >> used to define size of the stats counter. > >> "uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];" > >> > >> When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors, > >> the one in the ethdev is like [1]. > >> > >> This can be fixed two ways, > >> a) increase the size of 'stat_idx' storage type to u16 in the > >> '*_queue_stats_mapping' APIs, this is what this patch does. > >> b) Fix with a casting in the comparison, without changing the APIs. > >> > >> I think both are OK, but is (b) more preferable? > > > > I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is > > not modified. > > > > On the substance, I agree with Thomas that the queue_stats_mapping API > > should be replaced by xstats. > > > > This has been discussed in the last technical board meeting, the decision was > to > use xstats to get queue related statistics [2]. > > But after second look, even if xstats is used to get statistics, > 'RTE_ETHDEV_QUEUE_STAT_CNTRS' is used, since xstats uses > 'rte_eth_stats_get()' > to get queue statistics. > So for the case device has more than 255 queues, > 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > still needs to be set > 255 which will cause the build error.
You're right, when using the old API in xstats implementation, we are limited to RTE_ETHDEV_QUEUE_STAT_CNTRS queues. > I have an AR to send a deprecation notice to current method to get the queue > statistics, and limit the old method to 256 queues. But since xstats is just > a > wrapped to old method, I am not quite sure how deprecating it will work. > > @Thomas, @Honnappa, can you give some more insight on the issue? It becomes a PMD issue. The PMD implementation of xstats must complete the statistics for the queues above RTE_ETHDEV_QUEUE_STAT_CNTRS. In order to prepare the removal of the old method smoothly, we could add a driver flag which indicates whether the PMD relies on a pre-fill of xstats from old stats per queue conversion or not. > [2] > https://mails.dpdk.org/archives/dev/2020-October/185299.html
