Re: [PATCH v3 1/3] add new xstats API retrieving by id

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2017-04-04 15:45, Van Haaren, Harry:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2017-04-03 14:09, Jacek Piasecki:
> > > From: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
> > >
> > > Extended xstats API in ethdev library to allow grouping of stats
> > > logically so they can be retrieved per logical grouping – managed
> > > by the application.
> > > Changed existing functions rte_eth_xstats_get_names and
> > > rte_eth_xstats_get to use a new list of arguments: array of ids
> > > and array of values. ABI versioning mechanism was used to
> > > support backward compatibility.
> > > Introduced two new functions rte_eth_xstats_get_all and
> > > rte_eth_xstats_get_names_all which keeps functionality of the
> > > previous ones (respectively rte_eth_xstats_get and
> > > rte_eth_xstats_get_names) but use new API inside.
> > 
> > Sorry, I still do not understand why we should complicate the API.
> > What is not possible with the existing API?
> 
> 
> The current API only allows retrieval of *all* of the NIC statistics at once. Given this requires a MMIO read PCI transaction per statistic it is an inefficient way of retrieving just a few key statistics. My understanding is that often a monitoring agent only has an interest in a few key statistics, and the current API forces wasting CPU time and PCIe bandwidth in retrieving *all* statistics; even those that the application didn't explicitly show an interest in.
> 
> The more flexible API as implemented in this patchset allow retrieval of statistics per ID. If a PMD wishes, it can be implemented to read just the required NIC registers. As a result, the monitoring application no longer wastes PCIe bandwidth and CPU time.

Thanks for the explanation.
It has never been explained before.

> > The v1 was submitted in the last days of the proposal deadline,
> > v2 in the last minutes of integration deadline,
> > and v3 is submitted after the deadline.
> > 
> > Given it is late and it is still difficult to understand the benefit,
> > I think it won't make the release 17.05.
> 
> 
> All in all, the value add to DPDK of this patchset is to enable applications request statistics of interest to them, and to allow PMDs implement the statistic functions more efficiently if they wish. As a bonus, the ethdev and eventdev xstats APIs will have a consistent design, as eventdev already uses this optimized ID based method.
> 
> Unless there are serious concerns about the current API (which should have been flagged between a v1 and now), I don't see a reason to not update the API to use this improved method. If there are concerns about how to update applications to the new API, that can be addressed in a documentation patch if the community feels there is value in that?

I have commented on the need of explanation 3 days after the v1.
There was no answer.
So the review stopped at this point.
Then one month later (last Thursday), a v2 appears which
"replaced grouping mechanism to use mechanism based on IDs".
So you cannot say it "should have been flagged between a v1 and now".

Just because of the lack of communication, I do not want to spend these
days reviewing the API. It needs time and it will wait.