From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David Harton (dharton)" Subject: Re: [RFC PATCH v1 0/3] Remove string operations from xstats Date: Thu, 28 Apr 2016 15:58:37 +0000 Message-ID: References: <1460731462-21229-1-git-send-email-remy.horton@intel.com> <1A27633A6DA49C4A92FCD5D4312DBF536B1814BF@IRSMSX106.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Mcnamara, John" , "Van Haaren, Harry" To: "Tahhan, Maryam" , "Horton, Remy" , "dev@dpdk.org" Return-path: Received: from alln-iport-5.cisco.com (alln-iport-5.cisco.com [173.37.142.92]) by dpdk.org (Postfix) with ESMTP id 48791558A for ; Thu, 28 Apr 2016 17:58:39 +0200 (CEST) In-Reply-To: <1A27633A6DA49C4A92FCD5D4312DBF536B1814BF@IRSMSX106.ger.corp.intel.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Tahhan, Maryam [mailto:maryam.tahhan@intel.com] > Sent: Thursday, April 28, 2016 10:56 AM > To: David Harton (dharton) ; Horton, Remy > ; dev@dpdk.org > Cc: Mcnamara, John ; Van Haaren, Harry > > Subject: RE: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > xstats >=20 > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Harton > > (dharton) > > Sent: Wednesday, April 20, 2016 5:04 PM > > To: Horton, Remy ; dev@dpdk.org > > Subject: Re: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations > > from xstats > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton > > > Sent: Friday, April 15, 2016 10:44 AM > > > To: dev@dpdk.org > > > Subject: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > > > xstats > > > > > > The current extended ethernet statistics fetching involve doing > > > several string operations, which causes performance issues if there > > > are lots of statistics and/or network interfaces. This RFC patchset > > > changes the API for xstats to use integer identifiers instead of > > > strings and implements this new API for the ixgbe driver. Others > > drivers to follow. > > > > > > -- > > > > > > Since this will involve API & ABI breakage as previously advertised, > > > there are several design assumptions that need consideration: > > > > > > *) id-name & id-value pairs for both lookup and query Permits > > > out-of-order and non-contigious returning of names/ids/values, even > > > though expected implmentations would in practice return items in > > > sorted order by id. Is this sufficent/desirable future proofing? > > > Idea is to allow possibility of drivers returning partial statistics. > > > > I believe forcing drivers to match to a common id-space will become > > burdensome. If the stats id-space isn't common then matching strings > > is probably just as sufficient as long as drivers don't add/remove > > stats ad hoc between the time the device is initialized and removed. >=20 > I'm not aware of drivers adding/removing the stats ad hoc? The idea is to > have a common-id space otherwise it will be a free for all and we won't > have alignment across the drivers. I don't see it being any more > burdensome than having a common register naming across the board which is > what is there today. The advantage being that you don't have to pull the > strings every time. >=20 > > > > > > > > *) Bulk name-id mapping lookup only > > > At the moment individual lookup is not supported, as this would > > > impose extra overheads on drivers. The assumption is that any end > > > user would fetch all this data once on startup and then cache the > mappings. > > > > I'm not sure I see the value of looking up a single stat from a user > > perspective. I can see where the drivers might say that some stats > > are less disruptive/etc but the user doesn't have that knowledge and > > wouldn't know how to take advantage. Usually all stats are grabbed > > multiple times and the changes noted during debug sessions. > > >=20 > I believe Remy's change doesn't suggest/support individual lookup. It is > just a statement that we don't want to burden drivers with individual > stats lookups. >=20 > > > > > > *) Replacement or additional API > > > This patch replaces the current xstats API, but there is no inherant > > > reason beyond maintainability why this funtionality could not be in > > > addition rather than a replacement. What is consensus on this? > > > > I came to the conclusion that replacing the existing API isn't > > necessary but rather extending it so backwards compatibility could be > > maintained during the previous discussions on this topic. However, if > > we want to go forward with cleaning up in order to reduce the support > > drivers provide I'm all for it. > > > > I still believe the API we develop should follow an "ethtool stats like= " > > format as suggested earlier this year: > > > > extern int rte_eth_xstats_names_get(uint8_t port_id, > > struct rte_eth_xstats_name *names, unsigned n); extern int > > rte_eth_xstats_values_get(uint8_t port_id, > > uint64_t *values, unsigned n); > > > > Again, these could be provided alongside the existing API or replace it= . >=20 > I'm struggling a bit here. This is really what Remy has posted > http://dpdk.org/dev/patchwork/patch/12094/ or am I missing something > obvious? Maybe I misread the patch series or missed one but I don't see where=20 stats can be obtained without copying strings? This is the real issue I=20 raised originally. =20 Having the ability to get the names without stats is useful, but, the real gain would be obtaining the stats without the names. >=20 > > > > I also like the idea you provided of a separate API to obtain the > > xstats count rather than deriving the count by calling one of the > > above functions with "dummy" values. >=20 > +1 >=20 > > > > Again, I can provide the patches for the changes I've made that align > > with this proposed API. I just never got any feedback on it when > > requested previously. >=20 > I believe time is not in our favour on this front. If you have patches ca= n > you post them, otherwise can you please review the patchset that Remy has > posted? I'm working on it but I have some process I'm navigating. I'm hopeful=20 I'll have the green light within a week if not sooner. I apologize... I'm pushing as hard as I can. If you need to proceed go ahead I=20 completely understand. =20 All I can say is that I have implemented the API above and converted all=20 drivers that supported xstats in v2.2. Any new drivers that have added=20 xstats support since would need to be added. I did not add "get the count" because it wasn't provided in the current API and instead followed the convention but I do believe overtly getting the count it is the better approach. Thanks, Dave