2017-04-04 15:45, Van Haaren, Harry: > From: Thomas Monjalon [mailto:thomas.monja...@6wind.com] > > 2017-04-03 14:09, Jacek Piasecki: > > > From: Michal Jastrzebski <michalx.k.jastrzeb...@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.
