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

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 12 Apr 2019 16:32:01 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Fri, Apr 12, 2019 at 3:29 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 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.
> >  
> 
> It will be quite artificial, but I can do this yes.
> 
> 
> > > > 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.
> >  
> 
> Ok, so I will describe this as a "driver interface" update.
> 
> 
> 
> > > 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.
> >  
> 
> That's a different topic from my pov, but yes, this mapping stuff should go
> away, later.
> 
> 
> > > 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.
> >  
> 
> Since the time I had posted this RFC, I have worked on a RFC v2, I will
> post this next week, with the drivers I found time to convert.
> We will have to take a decision on what goes to -rc2 between this and the
> q_errors[] patchset.
> 
> 

It looks like this all about maintaining source compatiablity with older
or out of tree drivers. This is not something DPDK has to worry about.
Why not just do a mondo patch that fixes all the drivers to use the new stats API.
You do need to keep the same ethdev interface for applications, but driver
API can change.