> -----Original Message----- > From: Tahhan, Maryam [mailto:maryam.tahhan at intel.com] > Sent: Thursday, April 28, 2016 10:56 AM > To: David Harton (dharton) <dharton at cisco.com>; Horton, Remy > <remy.horton at intel.com>; dev at dpdk.org > Cc: Mcnamara, John <john.mcnamara at intel.com>; Van Haaren, Harry > <harry.van.haaren at intel.com> > Subject: RE: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > xstats > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of David Harton > > (dharton) > > Sent: Wednesday, April 20, 2016 5:04 PM > > To: Horton, Remy <remy.horton at intel.com>; dev at dpdk.org > > Subject: Re: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations > > from xstats > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton > > > Sent: Friday, April 15, 2016 10:44 AM > > > To: dev at 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. > > 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. > > > > > > > > > *) 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. > > > > 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. > > > > > > > *) 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. > > 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 stats can be obtained without copying strings? This is the real issue I raised originally. Having the ability to get the names without stats is useful, but, the real gain would be obtaining the stats without the names. > > > > > 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. > > +1 > > > > > 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. > > I believe time is not in our favour on this front. If you have patches can > 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 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 completely understand. All I can say is that I have implemented the API above and converted all drivers that supported xstats in v2.2. Any new drivers that have added 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
