DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas.monjalon@6wind.com>
To: "Van Haaren, Harry" <harry.van.haaren@intel.com>
Cc: dev@dpdk.org, "Kozak, KubaX" <kubax.kozak@intel.com>,
	"Kulasek, TomaszX" <tomaszx.kulasek@intel.com>,
	"Piasecki, JacekX" <jacekx.piasecki@intel.com>,
	"Jastrzebski, MichalX K" <michalx.k.jastrzebski@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3 1/3] add new xstats API retrieving by id
Date: Tue, 04 Apr 2017 18:18:49 +0200	[thread overview]
Message-ID: <6167487.QzSI3n9msi@xps13> (raw)
In-Reply-To: <E923DB57A917B54B9182A2E928D00FA612A28F4A@IRSMSX102.ger.corp.intel.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.

  reply	other threads:[~2017-04-04 16:18 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-30 21:50 [dpdk-dev] [PATCH v2 0/5] Extended xstats API in ethdev library to allow grouping of stats Michal Jastrzebski
2017-03-30 21:50 ` [dpdk-dev] [PATCH v2 1/5] add new xstats API retrieving by id Michal Jastrzebski
2017-04-03 12:09   ` [dpdk-dev] [PATCH v3 0/3] Extended xstats API in ethdev library to allow grouping of stats Jacek Piasecki
2017-04-03 12:09     ` [dpdk-dev] [PATCH v3 1/3] add new xstats API retrieving by id Jacek Piasecki
2017-04-03 12:37       ` Van Haaren, Harry
2017-04-04 15:03       ` Thomas Monjalon
2017-04-04 15:45         ` Van Haaren, Harry
2017-04-04 16:18           ` Thomas Monjalon [this message]
2017-04-10 17:59       ` [dpdk-dev] [PATCH v4 0/3] Extended xstats API in ethdev library to allow grouping of stats Jacek Piasecki
2017-04-10 17:59         ` [dpdk-dev] [PATCH v4 1/3] ethdev: new xstats API add retrieving by ID Jacek Piasecki
2017-04-11 16:37           ` [dpdk-dev] [PATCH v5 0/3] Extended xstats API in ethdev library to allow grouping of stats Michal Jastrzebski
2017-04-11 16:37             ` [dpdk-dev] [PATCH v5 1/3] ethdev: new xstats API add retrieving by ID Michal Jastrzebski
2017-04-12  8:56               ` Van Haaren, Harry
2017-04-12 17:10               ` Thomas Monjalon
2017-04-13 10:52               ` Mcnamara, John
2017-04-13 14:59               ` [dpdk-dev] [PATCH v6 0/5] Extended xstats API in ethdev library to allow grouping of stats Kuba Kozak
2017-04-13 14:59                 ` [dpdk-dev] [PATCH v6 1/5] ethdev: new xstats API add retrieving by ID Kuba Kozak
2017-04-13 16:23                   ` Van Haaren, Harry
2017-04-13 14:59                 ` [dpdk-dev] [PATCH v6 2/5] ethdev: added new function for xstats ID Kuba Kozak
2017-04-13 16:23                   ` Van Haaren, Harry
2017-04-13 14:59                 ` [dpdk-dev] [PATCH v6 3/5] proc-info: add support for new xstats API Kuba Kozak
2017-04-13 16:23                   ` Van Haaren, Harry
2017-04-13 14:59                 ` [dpdk-dev] [PATCH v6 4/5] net/e1000: new xstats API add ID support for e1000 Kuba Kozak
2017-04-13 16:23                   ` Van Haaren, Harry
2017-04-13 14:59                 ` [dpdk-dev] [PATCH v6 5/5] net/ixgbe: new xstats API add ID support for ixgbe Kuba Kozak
2017-04-13 16:23                   ` Van Haaren, Harry
2017-04-13 16:21                 ` [dpdk-dev] [PATCH v6 0/5] Extended xstats API in ethdev library to allow grouping of stats Van Haaren, Harry
2017-04-20 20:31                 ` Thomas Monjalon
2017-04-24 12:32                   ` Olivier Matz
2017-04-24 12:41                     ` Thomas Monjalon
2017-04-24 15:49                       ` Mcnamara, John
2017-04-25 22:49                         ` Roger B Melton
2017-04-11 16:37             ` [dpdk-dev] [PATCH v5 2/3] net/e1000: new xstats API add ID support for e1000 Michal Jastrzebski
2017-04-12  8:56               ` Van Haaren, Harry
2017-04-11 16:37             ` [dpdk-dev] [PATCH v5 3/3] net/ixgbe: new xstats API add ID support for ixgbe Michal Jastrzebski
2017-04-12  8:56               ` Van Haaren, Harry
2017-04-10 17:59         ` [dpdk-dev] [PATCH v4 2/3] net/e1000: new xstats API add ID support for e1000 Jacek Piasecki
2017-04-10 17:59         ` [dpdk-dev] [PATCH v4 3/3] net/ixgbe: new xstats API add ID support for ixgbe Jacek Piasecki
2017-04-03 12:09     ` [dpdk-dev] [PATCH v3 2/3] add new xstats API id support for e1000 Jacek Piasecki
2017-04-03 12:38       ` Van Haaren, Harry
2017-04-03 12:09     ` [dpdk-dev] [PATCH v3 3/3] add new xstats API id support for ixgbe Jacek Piasecki
2017-04-03 12:38       ` Van Haaren, Harry
2017-03-30 21:50 ` [dpdk-dev] [PATCH v2 2/5] add new xstats API id support for e1000 Michal Jastrzebski
2017-03-30 22:06 ` [dpdk-dev] [PATCH v2 3/5] add new xstats API id support for ixgbe Michal Jastrzebski
2017-03-30 22:22 ` [dpdk-dev] [PATCH v2 4/5] add support for new xstats API retrieving by id Michal Jastrzebski
2017-03-30 22:23 ` [dpdk-dev] [PATCH v2 5/5] " Michal Jastrzebski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6167487.QzSI3n9msi@xps13 \
    --to=thomas.monjalon@6wind.com \
    --cc=dev@dpdk.org \
    --cc=harry.van.haaren@intel.com \
    --cc=jacekx.piasecki@intel.com \
    --cc=kubax.kozak@intel.com \
    --cc=michalx.k.jastrzebski@intel.com \
    --cc=tomaszx.kulasek@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).