From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v3 1/3] add new xstats API retrieving by id Date: Tue, 04 Apr 2017 18:18:49 +0200 Message-ID: <6167487.QzSI3n9msi@xps13> References: <1490910640-244285-2-git-send-email-michalx.k.jastrzebski@intel.com> <2943087.L4hhIMZ3WY@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org, "Kozak, KubaX" , "Kulasek, TomaszX" , "Piasecki, JacekX" , "Jastrzebski, MichalX K" To: "Van Haaren, Harry" Return-path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 6B22C326C for ; Tue, 4 Apr 2017 18:18:51 +0200 (CEST) Received: by mail-wm0-f42.google.com with SMTP id o81so29860761wmb.1 for ; Tue, 04 Apr 2017 09:18:51 -0700 (PDT) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > > > > > Extended xstats API in ethdev library to allow grouping of stats > > > logically so they can be retrieved per logical grouping =E2=80=93= 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. > >=20 > > Sorry, I still do not understand why we should complicate the API. > > What is not possible with the existing API? >=20 >=20 > 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 un= derstanding is that often a monitoring agent only has an interest in a = few key statistics, and the current API forces wasting CPU time and PCI= e bandwidth in retrieving *all* statistics; even those that the applica= tion didn't explicitly show an interest in. >=20 > 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 j= ust 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. > >=20 > > Given it is late and it is still difficult to understand the benefi= t, > > I think it won't make the release 17.05. >=20 >=20 > All in all, the value add to DPDK of this patchset is to enable appli= cations request statistics of interest to them, and to allow PMDs imple= ment 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. >=20 > 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 u= pdate 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 d= ocumentation 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.