Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Thomas Monjalon <thomas@monjalon.net>]

26/03/2019 10:29, David Marchand:
> On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 3/14/2019 3:13 PM, David Marchand wrote:
> > > Introduce a new api to retrieve per queue statistics from the drivers.
> > > The api objectives:
> > > - easily add some common per queue statistics and have it exposed
> > >   through the user xstats api while the user stats api is left untouched
> > > - remove the limitations on the per queue statistics count (inherited
> > >   from ixgbe) and avoid recurrent bugs on stats array overflow

First comment, I think it would be easier to read if renaming the legacy
basic stats interface was in a separate patch.

> > The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
> > concern is if it is overkill to have three dev_ops to get stats
> > and I am feeling that is making xstat code more complex.
> 
> Having these new (meant to be) internal dev_ops has the avantage of
> separating the statistics reported from the drivers from the exported api.
> This is also why I did not prefix the structure names with rte_.

Yes, and to make it clear, please do not talk about API,
as it is only a driver interface.

> The "complex" part is in a single place, ethdev and this is when
> translating from an internal representation to the exposed bits in the
> public apis.
> 
> Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct
> > rte_eth_stats'?
> >
> 
> It does not solve the problem of drivers that are buggy because of the
> limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
> All drivers need to be aware of this limitation of the rte_eth_stats
> structure.

Yes, this limitation should be dropped.
I would like to see the functions rte_eth_dev_set_?x_queue_stats_mapping()
deprecated as they were a bad abstraction of ixgbe limitation.

> The drivers manipulate queues in rx/tx_queue_setup dev_ops for example,
> having rxq/txq_stats_get dev_ops is more consistent to me.

+1

> > And perhaps we can do the 'fix rxq q_errors' patchset [1] after this
> > change, so
> > fix can be done with less changes, although it will push the fix into next
> > release because of the ABI break.
> 
> I am fine with merging this together, we don't want to backport this
> anyway, right?

No, it would make some behaviours changing in stable releases,
so better to not backport it and keep the buggy behaviour in old branches.

> But so far, I don't feel the need to break the rte_eth_stats_get API, if we
> want to go to this, we can define an entirely new api to expose
> standardized bits and still use the rxq/txq_stats_get internal dev_ops I
> propose.

Good