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

[David Marchand <david.marchand@redhat.com>]

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.


-- 
David Marchand