Re: [RFC PATCH v1 0/3] Remove string operations from xstats

["David Harton (dharton)" <dharton@cisco.com>]

> -----Original Message-----
> From: Tahhan, Maryam [mailto:maryam.tahhan@intel.com]
> Sent: Thursday, April 28, 2016 10:56 AM
> To: David Harton (dharton) <dharton@cisco.com>; Horton, Remy
> <remy.horton@intel.com>; dev@dpdk.org
> Cc: Mcnamara, John <john.mcnamara@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: RE: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from
> xstats
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Harton
> > (dharton)
> > Sent: Wednesday, April 20, 2016 5:04 PM
> > To: Horton, Remy <remy.horton@intel.com>; 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.
> 
> 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