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

[Thomas Monjalon <thomas@monjalon.net>]

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 <thomas@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 <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.
> >>>>
> >>>> 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