26/03/2019 10:29, David Marchand: > On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yi...@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
