[dpdk-dev] Future Direction for rte_eth_stats_get()

[dpdk-dev] Future Direction for rte_eth_stats_get()

[David Harton (dharton)]

I see that some of the rte_eth_stats have been marked deprecated in 2.2 that are returned by rte_eth_stats_get().  Applications that utilize any number of device types rely on functions like this one to debug I/O issues.

Is there a reason the stats have been deprecated?  Why not keep the stats in line with the standard linux practices such as rtnl_link_stats64?

Note, using rte_eth_xstats_get() does not help for this particular scenario because a common binary API is needed to communicate through various layers and also provide a consistent view/meaning to users.  The xstats is excellent for debugging device specific scenarios but can't help in scenarios where a static view is expected.

Thanks,
Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Tahhan, Maryam]

Hi David
Some of the stats were HW specific rather than generic stats that should be exposed through rte_eth_stats and were migrated to the xstats API. http://dpdk.org/ml/archives/dev/2015-June/019915.html. The naming was also not generic enough to cover some of the drivers and in some cases what was exposed was only a partial view of the relevant stats.

They were marked as deprecated to deter people from using them in the future, but haven't been removed from all the driver implementations yet. The Registers that remain undeprecated are those considered to be generic.

Which registers are you particularly interested in that have been deprecated? Can you elaborate on what you mean by " scenarios where a static view is expected "

Thanks in advance.

Best Regards, 
Maryam


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Harton
> (dharton)
> Sent: Wednesday, January 20, 2016 5:19 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] Future Direction for rte_eth_stats_get()
> 
> I see that some of the rte_eth_stats have been marked deprecated in 2.2
> that are returned by rte_eth_stats_get().  Applications that utilize any
> number of device types rely on functions like this one to debug I/O
> issues.
> 
> Is there a reason the stats have been deprecated?  Why not keep the
> stats in line with the standard linux practices such as rtnl_link_stats64?
> 
> Note, using rte_eth_xstats_get() does not help for this particular scenario
> because a common binary API is needed to communicate through
> various layers and also provide a consistent view/meaning to users.  The
> xstats is excellent for debugging device specific scenarios but can't help
> in scenarios where a static view is expected.
> 
> Thanks,
> Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[David Harton (dharton)]

Hi Maryam,

Thanks for the pointer.  I'll review the convo's.

Consider I have an application that uses many(all?) of the DPDK drivers and their netmap counterparts depending on configuration.  The user interface provides a set of I/O stats to help debug I/O issues and that set is the same regardless of driver type.  The set of stats provided matches what linux provides today since netmap existed before dpdk.

What I want to avoid is having an application that is driver independent having to become driver dependent interpreting a bunch of strings (from xstats) or worse the layer running at the data plane core that is advertising the API needed by the application parsing those strings because the application cannot change the upper layer.

What if instead of passing strings and values a set of stat ids and value are returned.  At least this way the application can remain driver agnostic versus having to parse a free form string that likely isn't the same across driver types.

Also, why wouldn't rte_eth_stats_get() align with linux which is the defacto standard?  I understand that not every driver may not support every stat but that's ok.  Just return 0 (pause frames, crc errors, etc).  It's not like the list of linux stats is ever growing or advertising an absurd number of stats that aren't applicable to all drivers.

Thanks again,
Dave

> -----Original Message-----
> From: Tahhan, Maryam [mailto:maryam.tahhan@intel.com]
> Sent: Friday, January 22, 2016 6:08 AM
> To: David Harton (dharton) <dharton@cisco.com>; dev@dpdk.org
> Subject: RE: Future Direction for rte_eth_stats_get()
> 
> Hi David
> Some of the stats were HW specific rather than generic stats that should
> be exposed through rte_eth_stats and were migrated to the xstats API.
> http://dpdk.org/ml/archives/dev/2015-June/019915.html. The naming was also
> not generic enough to cover some of the drivers and in some cases what was
> exposed was only a partial view of the relevant stats.
> 
> They were marked as deprecated to deter people from using them in the
> future, but haven't been removed from all the driver implementations yet.
> The Registers that remain undeprecated are those considered to be generic.
> 
> Which registers are you particularly interested in that have been
> deprecated? Can you elaborate on what you mean by " scenarios where a
> static view is expected "
> 
> Thanks in advance.
> 
> Best Regards,
> Maryam
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Harton
> > (dharton)
> > Sent: Wednesday, January 20, 2016 5:19 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] Future Direction for rte_eth_stats_get()
> >
> > I see that some of the rte_eth_stats have been marked deprecated in 2.2
> > that are returned by rte_eth_stats_get().  Applications that utilize any
> > number of device types rely on functions like this one to debug I/O
> > issues.
> >
> > Is there a reason the stats have been deprecated?  Why not keep the
> > stats in line with the standard linux practices such as
> rtnl_link_stats64?
> >
> > Note, using rte_eth_xstats_get() does not help for this particular
> scenario
> > because a common binary API is needed to communicate through
> > various layers and also provide a consistent view/meaning to users.  The
> > xstats is excellent for debugging device specific scenarios but can't
> help
> > in scenarios where a static view is expected.
> >
> > Thanks,
> > Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Tahhan, Maryam]

Hi David

+ Olivier and Harry as contributors and advisors in the past on the stats and stats API.

So I pulled the rtnl_link_stats64 from http://lxr.free-electrons.com/source/include/uapi/linux/if_link.h#L41 and crossed them with http://dpdk.org/dev/patchwork/patch/5842/ and http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.h 

40 /* The main device statistics structure */
 41 struct rtnl_link_stats64 {
 42         __u64   rx_packets;             /* total packets received       */             <MT> is in struct rte_eth_stats
 43         __u64   tx_packets;             /* total packets transmitted    */          <MT> is in struct rte_eth_stats
 44         __u64   rx_bytes;               /* total bytes received         */                  <MT> is in struct rte_eth_stats
 45         __u64   tx_bytes;               /* total bytes transmitted      */               <MT> is in struct rte_eth_stats
 46         __u64   rx_errors;              /* bad packets received         */                <MT> I'm working on a patch to distinguish between error and drops for struct rte_eth_stats at the moment only drops are reported through ierrors and oerrors and we are tied to those field name for backward compatibility.
 47         __u64   tx_errors;              /* packet transmit problems     */           <MT> Same comment as rx_errors.
 48         __u64   rx_dropped;             /* no space in linux buffers    */          <MT> is in struct rte_eth_stats
 49         __u64   tx_dropped;             /* no space available in linux  */         <MT> is in struct rte_eth_stats
 50         __u64   multicast;              /* multicast packets received   */           <MT> was deprecated in struct rte_eth_stats - but we can remove the notice and expose again from drivers we modified
 51         __u64   collisions; <MT> Not in struct rte_eth_stats - not sure it's in xstats either... 
 52 
 53         /* detailed rx_errors: */
 54         __u64   rx_length_errors;                                                                            <MT> was deprecated in struct rte_eth_stats, is in xstats - but we can remove the notice and expose again from drivers we modified
 55         __u64   rx_over_errors;         /* receiver ring buff overflow  */    <MT> was never exposed to struct rte_eth_stats  - but is in xstats.
 56         __u64   rx_crc_errors;          /* recved pkt with crc error    */         <MT> was deprecated in struct rte_eth_stats, is in xstats - but we can remove the notice and expose again from drivers we modified
 57         __u64   rx_frame_errors;        /* recv'd frame alignment error */ <MT> was never exposed to struct rte_eth_stats - but is in xstats.
 58         __u64   rx_fifo_errors;         /* recv'r fifo overrun          */               <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 59         __u64   rx_missed_errors;       /* receiver missed packet       */   <MT> was deprecated in struct rte_eth_stats, is in xstats - but we can remove the notice
 60 
 61         /* detailed tx_errors */
 62         __u64   tx_aborted_errors;                                                                         <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 63         __u64   tx_carrier_errors;                                                                            <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 64         __u64   tx_fifo_errors;                                                                                  <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 65         __u64   tx_heartbeat_errors;                                                                     <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 66         __u64   tx_window_errors;                                                                         <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 67 
 68         /* for cslip etc */
 69         __u64   rx_compressed;                                                                               <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 70         __u64   tx_compressed;                                                                               <MT> was never in struct rte_eth_stats. Not exposed by all drivers
 71 };

So what can be enabled again in struct rte_eth_stats  from what was already there is the equivalent of: 
* rx_length_errors
* rx_crc_errors
* rx_missed_errors - the deprecation notice was removed for this field.
* multicast

What should be added in to distinguish between errors and drops. struct rte_eth_stats :
 * rx_errors
 * tx_errors

As for the detailed rx errors and tx errors I'm open to feedback from you folks as to what should go in and what is too detailed. These weren't in struct rte_eth_stats previously, they are available through xstats and are uniformly named across the drivers. Oliver + Harry any thoughts?

David I assume you are looking for all the missing fields to be added?

Best Regards, 
Maryam


> -----Original Message-----
> From: David Harton (dharton) [mailto:dharton@cisco.com]
> Sent: Friday, January 22, 2016 1:41 PM
> To: Tahhan, Maryam <maryam.tahhan@intel.com>; dev@dpdk.org
> Subject: RE: Future Direction for rte_eth_stats_get()
> 
> Hi Maryam,
> 
> Thanks for the pointer.  I'll review the convo's.
> 
> Consider I have an application that uses many(all?) of the DPDK drivers
> and their netmap counterparts depending on configuration.  The user
> interface provides a set of I/O stats to help debug I/O issues and that
> set is the same regardless of driver type.  The set of stats provided
> matches what linux provides today since netmap existed before dpdk.
> 
> What I want to avoid is having an application that is driver independent
> having to become driver dependent interpreting a bunch of strings
> (from xstats) or worse the layer running at the data plane core that is
> advertising the API needed by the application parsing those strings
> because the application cannot change the upper layer.
> 
> What if instead of passing strings and values a set of stat ids and value
> are returned.  At least this way the application can remain driver
> agnostic versus having to parse a free form string that likely isn't the
> same across driver types.
> 
> Also, why wouldn't rte_eth_stats_get() align with linux which is the
> defacto standard?  I understand that not every driver may not support
> every stat but that's ok.  Just return 0 (pause frames, crc errors, etc).
> It's not like the list of linux stats is ever growing or advertising an
> absurd number of stats that aren't applicable to all drivers.
> 
> Thanks again,
> Dave
> 
> > -----Original Message-----
> > From: Tahhan, Maryam [mailto:maryam.tahhan@intel.com]
> > Sent: Friday, January 22, 2016 6:08 AM
> > To: David Harton (dharton) <dharton@cisco.com>; dev@dpdk.org
> > Subject: RE: Future Direction for rte_eth_stats_get()
> >
> > Hi David
> > Some of the stats were HW specific rather than generic stats that
> > should be exposed through rte_eth_stats and were migrated to the
> xstats API.
> > http://dpdk.org/ml/archives/dev/2015-June/019915.html. The
> naming was
> > also not generic enough to cover some of the drivers and in some
> cases
> > what was exposed was only a partial view of the relevant stats.
> >
> > They were marked as deprecated to deter people from using them
> in the
> > future, but haven't been removed from all the driver
> implementations yet.
> > The Registers that remain undeprecated are those considered to be
> generic.
> >
> > Which registers are you particularly interested in that have been
> > deprecated? Can you elaborate on what you mean by " scenarios
> where a
> > static view is expected "
> >
> > Thanks in advance.
> >
> > Best Regards,
> > Maryam
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David
> Harton
> > > (dharton)
> > > Sent: Wednesday, January 20, 2016 5:19 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] Future Direction for rte_eth_stats_get()
> > >
> > > I see that some of the rte_eth_stats have been marked
> deprecated in
> > > 2.2 that are returned by rte_eth_stats_get().  Applications that
> > > utilize any number of device types rely on functions like this one
> > > to debug I/O issues.
> > >
> > > Is there a reason the stats have been deprecated?  Why not keep
> the
> > > stats in line with the standard linux practices such as
> > rtnl_link_stats64?
> > >
> > > Note, using rte_eth_xstats_get() does not help for this particular
> > scenario
> > > because a common binary API is needed to communicate through
> various
> > > layers and also provide a consistent view/meaning to users.  The
> > > xstats is excellent for debugging device specific scenarios but
> > > can't
> > help
> > > in scenarios where a static view is expected.
> > >
> > > Thanks,
> > > Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[David Harton (dharton)]

Hi Maryam,

I'm not dictating they be re-added (although adding would be nice) but more important I'm trying to express an application view point rather than a driver view point.  

I completely understand how a driver wants to be able to advertise all the stats they want to advertise to help them debug their issues (i.e. xstats).   Yet, I'm very interested in DPDK providing a driver agnostic method of advertising well-defined stats.

For example, what if there was a kind of "stats registry" composed of ID and name.  It would work similar to xtats except instead of advertising strings only the "get" API would return ID/count pairs.  If the application wishes to use the DPDK provided string they can call another API to get the stat string via the ID.  These IDs would be well-defined clearly explaining what the count represent.  This way the strings for counts will be uniform across drivers and also make it clear to the users what the counts truly represent and the application could obtain stats from any driver in a driver agnostic manner.

Just an idea,
Dave

> -----Original Message-----
> From: Tahhan, Maryam [mailto:maryam.tahhan@intel.com]
> Sent: Friday, January 22, 2016 9:18 AM
> To: David Harton (dharton) <dharton@cisco.com>; dev@dpdk.org
> Cc: Olivier MATZ <olivier.matz@6wind.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: RE: Future Direction for rte_eth_stats_get()
> 
> Hi David
> 
> + Olivier and Harry as contributors and advisors in the past on the stats
> and stats API.
> 
> So I pulled the rtnl_link_stats64 from http://lxr.free-
> electrons.com/source/include/uapi/linux/if_link.h#L41 and crossed them
> with http://dpdk.org/dev/patchwork/patch/5842/ and
> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.h
> 
> 40 /* The main device statistics structure */
>  41 struct rtnl_link_stats64 {
>  42         __u64   rx_packets;             /* total packets received
> */             <MT> is in struct rte_eth_stats
>  43         __u64   tx_packets;             /* total packets transmitted
> */          <MT> is in struct rte_eth_stats
>  44         __u64   rx_bytes;               /* total bytes received
> */                  <MT> is in struct rte_eth_stats
>  45         __u64   tx_bytes;               /* total bytes transmitted
> */               <MT> is in struct rte_eth_stats
>  46         __u64   rx_errors;              /* bad packets received
> */                <MT> I'm working on a patch to distinguish between error
> and drops for struct rte_eth_stats at the moment only drops are reported
> through ierrors and oerrors and we are tied to those field name for
> backward compatibility.
>  47         __u64   tx_errors;              /* packet transmit problems
> */           <MT> Same comment as rx_errors.
>  48         __u64   rx_dropped;             /* no space in linux buffers
> */          <MT> is in struct rte_eth_stats
>  49         __u64   tx_dropped;             /* no space available in linux
> */         <MT> is in struct rte_eth_stats
>  50         __u64   multicast;              /* multicast packets received
> */           <MT> was deprecated in struct rte_eth_stats - but we can
> remove the notice and expose again from drivers we modified
>  51         __u64   collisions; <MT> Not in struct rte_eth_stats - not
> sure it's in xstats either...
>  52
>  53         /* detailed rx_errors: */
>  54         __u64   rx_length_errors;
> <MT> was deprecated in struct rte_eth_stats, is in xstats - but we can
> remove the notice and expose again from drivers we modified
>  55         __u64   rx_over_errors;         /* receiver ring buff overflow
> */    <MT> was never exposed to struct rte_eth_stats  - but is in xstats.
>  56         __u64   rx_crc_errors;          /* recved pkt with crc error
> */         <MT> was deprecated in struct rte_eth_stats, is in xstats - but
> we can remove the notice and expose again from drivers we modified
>  57         __u64   rx_frame_errors;        /* recv'd frame alignment
> error */ <MT> was never exposed to struct rte_eth_stats - but is in
> xstats.
>  58         __u64   rx_fifo_errors;         /* recv'r fifo overrun
> */               <MT> was never in struct rte_eth_stats. Not exposed by
> all drivers
>  59         __u64   rx_missed_errors;       /* receiver missed packet
> */   <MT> was deprecated in struct rte_eth_stats, is in xstats - but we
> can remove the notice
>  60
>  61         /* detailed tx_errors */
>  62         __u64   tx_aborted_errors;
> <MT> was never in struct rte_eth_stats. Not exposed by all drivers
>  63         __u64   tx_carrier_errors;
> <MT> was never in struct rte_eth_stats. Not exposed by all drivers
>  64         __u64   tx_fifo_errors;
> <MT> was never in struct rte_eth_stats. Not exposed by all drivers
>  65         __u64   tx_heartbeat_errors;
> <MT> was never in struct rte_eth_stats. Not exposed by all drivers
>  66         __u64   tx_window_errors;
> <MT> was never in struct rte_eth_stats. Not exposed by all drivers
>  67
>  68         /* for cslip etc */
>  69         __u64   rx_compressed;
> <MT> was never in struct rte_eth_stats. Not exposed by all drivers
>  70         __u64   tx_compressed;
> <MT> was never in struct rte_eth_stats. Not exposed by all drivers
>  71 };
> 
> So what can be enabled again in struct rte_eth_stats  from what was
> already there is the equivalent of:
> * rx_length_errors
> * rx_crc_errors
> * rx_missed_errors - the deprecation notice was removed for this field.
> * multicast
> 
> What should be added in to distinguish between errors and drops. struct
> rte_eth_stats :
>  * rx_errors
>  * tx_errors
> 
> As for the detailed rx errors and tx errors I'm open to feedback from you
> folks as to what should go in and what is too detailed. These weren't in
> struct rte_eth_stats previously, they are available through xstats and are
> uniformly named across the drivers. Oliver + Harry any thoughts?
> 
> David I assume you are looking for all the missing fields to be added?
> 
> Best Regards,
> Maryam
> 
> 
> > -----Original Message-----
> > From: David Harton (dharton) [mailto:dharton@cisco.com]
> > Sent: Friday, January 22, 2016 1:41 PM
> > To: Tahhan, Maryam <maryam.tahhan@intel.com>; dev@dpdk.org
> > Subject: RE: Future Direction for rte_eth_stats_get()
> >
> > Hi Maryam,
> >
> > Thanks for the pointer.  I'll review the convo's.
> >
> > Consider I have an application that uses many(all?) of the DPDK drivers
> > and their netmap counterparts depending on configuration.  The user
> > interface provides a set of I/O stats to help debug I/O issues and that
> > set is the same regardless of driver type.  The set of stats provided
> > matches what linux provides today since netmap existed before dpdk.
> >
> > What I want to avoid is having an application that is driver independent
> > having to become driver dependent interpreting a bunch of strings
> > (from xstats) or worse the layer running at the data plane core that is
> > advertising the API needed by the application parsing those strings
> > because the application cannot change the upper layer.
> >
> > What if instead of passing strings and values a set of stat ids and
> value
> > are returned.  At least this way the application can remain driver
> > agnostic versus having to parse a free form string that likely isn't the
> > same across driver types.
> >
> > Also, why wouldn't rte_eth_stats_get() align with linux which is the
> > defacto standard?  I understand that not every driver may not support
> > every stat but that's ok.  Just return 0 (pause frames, crc errors,
> etc).
> > It's not like the list of linux stats is ever growing or advertising an
> > absurd number of stats that aren't applicable to all drivers.
> >
> > Thanks again,
> > Dave
> >
> > > -----Original Message-----
> > > From: Tahhan, Maryam [mailto:maryam.tahhan@intel.com]
> > > Sent: Friday, January 22, 2016 6:08 AM
> > > To: David Harton (dharton) <dharton@cisco.com>; dev@dpdk.org
> > > Subject: RE: Future Direction for rte_eth_stats_get()
> > >
> > > Hi David
> > > Some of the stats were HW specific rather than generic stats that
> > > should be exposed through rte_eth_stats and were migrated to the
> > xstats API.
> > > http://dpdk.org/ml/archives/dev/2015-June/019915.html. The
> > naming was
> > > also not generic enough to cover some of the drivers and in some
> > cases
> > > what was exposed was only a partial view of the relevant stats.
> > >
> > > They were marked as deprecated to deter people from using them
> > in the
> > > future, but haven't been removed from all the driver
> > implementations yet.
> > > The Registers that remain undeprecated are those considered to be
> > generic.
> > >
> > > Which registers are you particularly interested in that have been
> > > deprecated? Can you elaborate on what you mean by " scenarios
> > where a
> > > static view is expected "
> > >
> > > Thanks in advance.
> > >
> > > Best Regards,
> > > Maryam
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David
> > Harton
> > > > (dharton)
> > > > Sent: Wednesday, January 20, 2016 5:19 PM
> > > > To: dev@dpdk.org
> > > > Subject: [dpdk-dev] Future Direction for rte_eth_stats_get()
> > > >
> > > > I see that some of the rte_eth_stats have been marked
> > deprecated in
> > > > 2.2 that are returned by rte_eth_stats_get().  Applications that
> > > > utilize any number of device types rely on functions like this one
> > > > to debug I/O issues.
> > > >
> > > > Is there a reason the stats have been deprecated?  Why not keep
> > the
> > > > stats in line with the standard linux practices such as
> > > rtnl_link_stats64?
> > > >
> > > > Note, using rte_eth_xstats_get() does not help for this particular
> > > scenario
> > > > because a common binary API is needed to communicate through
> > various
> > > > layers and also provide a consistent view/meaning to users.  The
> > > > xstats is excellent for debugging device specific scenarios but
> > > > can't
> > > help
> > > > in scenarios where a static view is expected.
> > > >
> > > > Thanks,
> > > > Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Thomas Monjalon]

2016-01-22 14:18, Tahhan, Maryam:
> So what can be enabled again in struct rte_eth_stats  from what was already there is the equivalent of: 
> * rx_length_errors
> * rx_crc_errors
> * rx_missed_errors - the deprecation notice was removed for this field.
> * multicast
> 
> What should be added in to distinguish between errors and drops. struct rte_eth_stats :
>  * rx_errors
>  * tx_errors
> 
> As for the detailed rx errors and tx errors I'm open to feedback from you folks as to what should go in and what is too detailed. These weren't in struct rte_eth_stats previously, they are available through xstats and are uniformly named across the drivers. Oliver + Harry any thoughts?
> 
> David I assume you are looking for all the missing fields to be added?

They are not missing. They just not exactly match ones having a
long history in Linux kernel.
Please let's avoid to blindly mimic others without thinking
about modern needs.

> > > From: David Harton
> > > > Is there a reason the stats have been deprecated?  Why not keep
> > > > the stats in line with the standard linux practices such as
> > > > rtnl_link_stats64?

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Thomas Monjalon]

+ Harry

2016-01-22 14:40, David Harton:
> Hi Maryam,
> 
> I'm not dictating they be re-added (although adding would be nice) but more important I'm trying to express an application view point rather than a driver view point.  
> 
> I completely understand how a driver wants to be able to advertise all the stats they want to advertise to help them debug their issues (i.e. xstats).   Yet, I'm very interested in DPDK providing a driver agnostic method of advertising well-defined stats.

xstats are driver agnostic and have a well-defined naming scheme.

> For example, what if there was a kind of "stats registry" composed of ID and name.  It would work similar to xtats except instead of advertising strings only the "get" API would return ID/count pairs.  If the application wishes to use the DPDK provided string they can call another API to get the stat string via the ID.  These IDs would be well-defined clearly explaining what the count represent.  This way the strings for counts will be uniform across drivers and also make it clear to the users what the counts truly represent and the application could obtain stats from any driver in a driver agnostic manner.

I don't understand how adding another indirection (an ID matching a string)
would help?
I have the feeling you want a list of possible statistics, right?

PS: please avoid top posting

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Tahhan, Maryam]

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, January 22, 2016 2:44 PM
> To: Tahhan, Maryam <maryam.tahhan@intel.com>; David Harton
> (dharton) <dharton@cisco.com>
> Cc: dev@dpdk.org; olivier.matz@6wind.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()
> 
> 2016-01-22 14:18, Tahhan, Maryam:
> > So what can be enabled again in struct rte_eth_stats  from what was
> already there is the equivalent of:
> > * rx_length_errors
> > * rx_crc_errors
> > * rx_missed_errors - the deprecation notice was removed for this
> field.
> > * multicast
> >
> > What should be added in to distinguish between errors and drops.
> struct rte_eth_stats :
> >  * rx_errors
> >  * tx_errors
> >
> > As for the detailed rx errors and tx errors I'm open to feedback from
> you folks as to what should go in and what is too detailed. These
> weren't in struct rte_eth_stats previously, they are available through
> xstats and are uniformly named across the drivers. Oliver + Harry any
> thoughts?
> >
> > David I assume you are looking for all the missing fields to be added?
> 
> They are not missing. They just not exactly match ones having a long
> history in Linux kernel.
> Please let's avoid to blindly mimic others without thinking about
> modern needs.
> 

My bad wording - I apologise, but I agree we should consider what makes sense and what doesn't.

> > > > From: David Harton
> > > > > Is there a reason the stats have been deprecated?  Why not
> keep
> > > > > the stats in line with the standard linux practices such as
> > > > > rtnl_link_stats64?

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Igor Ryzhov]

Hello, everyone.

How about exposing stats according to IF-MIB?

Statistics to be exposed are - octets, unicast packets, multicast packets, broadcast packets, errors and discards for both TX and RX.
These counters are basic and implemented by most of drivers.

All other driver-specific counters can be exposed by xstats.

Best regards,
Igor

> 22 янв. 2016 г., в 17:44, Thomas Monjalon <thomas.monjalon@6wind.com> написал(а):
> 
> 2016-01-22 14:18, Tahhan, Maryam:
>> So what can be enabled again in struct rte_eth_stats  from what was already there is the equivalent of: 
>> * rx_length_errors
>> * rx_crc_errors
>> * rx_missed_errors - the deprecation notice was removed for this field.
>> * multicast
>> 
>> What should be added in to distinguish between errors and drops. struct rte_eth_stats :
>> * rx_errors
>> * tx_errors
>> 
>> As for the detailed rx errors and tx errors I'm open to feedback from you folks as to what should go in and what is too detailed. These weren't in struct rte_eth_stats previously, they are available through xstats and are uniformly named across the drivers. Oliver + Harry any thoughts?
>> 
>> David I assume you are looking for all the missing fields to be added?
> 
> They are not missing. They just not exactly match ones having a
> long history in Linux kernel.
> Please let's avoid to blindly mimic others without thinking
> about modern needs.
> 
>>>> From: David Harton
>>>>> Is there a reason the stats have been deprecated?  Why not keep
>>>>> the stats in line with the standard linux practices such as
>>>>> rtnl_link_stats64?
> 

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Van Haaren, Harry]

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()
 
> + Harry

Hey all,

> xstats are driver agnostic and have a well-defined naming scheme.

Indeed, described here:
http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html#extended-statistics-api

All of the rte_eth_stats statistics are presented in xstats consistently
(independent of which PMD is running), as they are implemented in rte_ethdev.c:
http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n1500


From: David Harton:
> For example, what if there was a kind of "stats registry" composed of ID and name.  It
> would work similar to xtats except instead of advertising strings only the "get" API would
> return ID/count pairs.  If the application wishes to use the DPDK provided string they can
> call another API to get the stat string via the ID.  These IDs would be well-defined
> clearly explaining what the count represent.  This way the strings for counts will be
> uniform across drivers and also make it clear to the users what the counts truly represent
> and the application could obtain stats from any driver in a driver agnostic manner.

What you (David Harton) describe about a well-defined name, and clearly explaining the value
it is representing is what the goal was for xstats: a human readable string, which can be
easily parsed and a value retrieved.

The "rules" of encoding a statistic as an xstats string are pretty simple, and if any PMD
breaks the rules, it should be considered broken and fixed.

Consistency is key, in this case consistency in the PMD xstats implementations to make it
useful for clients of the API.

The big advantage xstats has over adding items to rte_eth_stats is that it won't break ABI.

Do you see a fundamental problem with parsing the strings to retrieve values?

If you like I could code up a minimal sample-app that only pulls statistics,
and you can show "interest" in various statistics for printing / monitoring?


Regards, -Harry

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Jay Rolette]

On Fri, Jan 22, 2016 at 9:22 AM, Van Haaren, Harry <
harry.van.haaren@intel.com> wrote:

> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()
>
> > + Harry
>
> Hey all,
>
> > xstats are driver agnostic and have a well-defined naming scheme.
>
> Indeed, described here:
>
> http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html#extended-statistics-api
>
> All of the rte_eth_stats statistics are presented in xstats consistently
> (independent of which PMD is running), as they are implemented in
> rte_ethdev.c:
> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n1500
>
>
> From: David Harton:
> > For example, what if there was a kind of "stats registry" composed of ID
> and name.  It
> > would work similar to xtats except instead of advertising strings only
> the "get" API would
> > return ID/count pairs.  If the application wishes to use the DPDK
> provided string they can
> > call another API to get the stat string via the ID.  These IDs would be
> well-defined
> > clearly explaining what the count represent.  This way the strings for
> counts will be
> > uniform across drivers and also make it clear to the users what the
> counts truly represent
> > and the application could obtain stats from any driver in a driver
> agnostic manner.
>
> What you (David Harton) describe about a well-defined name, and clearly
> explaining the value
> it is representing is what the goal was for xstats: a human readable
> string, which can be
> easily parsed and a value retrieved.
>
> The "rules" of encoding a statistic as an xstats string are pretty simple,
> and if any PMD
> breaks the rules, it should be considered broken and fixed.
>
> Consistency is key, in this case consistency in the PMD xstats
> implementations to make it
> useful for clients of the API.
>
> The big advantage xstats has over adding items to rte_eth_stats is that it
> won't break ABI.
>
> Do you see a fundamental problem with parsing the strings to retrieve
> values?
>

When you have an appliance with a large number of ports and a large number
of stats that you need to collect, having to parse those strings constantly
is very slow compared to having fixed struct members or at least known ID
values.

Strings are also error prone. While they may be consistent at this point,
it is remarkably easy for them to get out of sync along the way. Using an
ID instead avoids that neatly.

Jay

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[David Harton (dharton)]

> > xstats are driver agnostic and have a well-defined naming scheme.
> 
> Indeed, described here:
> http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html#extended-
> statistics-api

Thanks for sharing.  I do think what is in the link is well thought out but it also may not match how the application would choose to represent that stat (capitalization, abbreviation, etc).

> From: David Harton:
> > For example, what if there was a kind of "stats registry" composed of
> > ID and name.  It would work similar to xtats except instead of
> > advertising strings only the "get" API would return ID/count pairs.
> > If the application wishes to use the DPDK provided string they can
> > call another API to get the stat string via the ID.  These IDs would
> > be well-defined clearly explaining what the count represent.  This way
> > the strings for counts will be uniform across drivers and also make it
> clear to the users what the counts truly represent and the application
> could obtain stats from any driver in a driver agnostic manner.
> 
> What you (David Harton) describe about a well-defined name, and clearly
> explaining the value it is representing is what the goal was for xstats: a
> human readable string, which can be easily parsed and a value retrieved.
> 
> The "rules" of encoding a statistic as an xstats string are pretty simple,
> and if any PMD breaks the rules, it should be considered broken and fixed.

I understand it may be broken, but that doesn't help finding them and ensuring they aren't broken to begin with.  What regression/automation is in place to ensure drivers aren't broken?

> 
> Consistency is key, in this case consistency in the PMD xstats
> implementations to make it useful for clients of the API.
> 
> The big advantage xstats has over adding items to rte_eth_stats is that it
> won't break ABI.

100% agree.  I can see the intent.

> 
> Do you see a fundamental problem with parsing the strings to retrieve
> values?

I think parsing strings is expensive CPU wise and again subject to error as devices are added.  That's why I was wondering if a binary id could be generated instead.  The binary id could also be used to look up the string name if the application wants it.  The API would be very similar to the current xstats API except it would pass id/value pairs instead of string/value pairs.  This avoids string comparisons to figure out while still allowing flexibility between drivers.  It would also 100% guarantee that any strings returned by "const char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent across devices.

I also think there is less chance for error if drivers assign their stats to ID versus constructing a string.  I haven't checked my application, but I wonder if the binary size would actually be smaller if all the stats strings were centrally located versus distributed across binaries (highly dependent on the linker and optimization level).

> 
> If you like I could code up a minimal sample-app that only pulls
> statistics, and you can show "interest" in various statistics for printing
> / monitoring?

Appreciate the offer.  I actually understand what's is available.  And, BTW, I apologize for being late to the game (looks like this was discussed last summer) but I'm just now getting looped in and following the mailer but I'm just wondering if something that is performance friendly for the user/application and flexible for the drivers is possible.

Thanks,
Dave

> 
> 
> Regards, -Harry

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Thomas Monjalon]

2016-01-22 16:04, David Harton:
> I think parsing strings is expensive CPU wise
[...]
> wondering if something that is performance friendly for the user/application
> and flexible for the drivers is possible.

I think we need some numbers from experimentations and some requirements.
How many cycles it takes to read stats?
How often stats must be read at max?

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Van Haaren, Harry]

+Jay, (@all, please keep everybody in the CCs :)

> From: David Harton (dharton) [mailto:dharton@cisco.com]
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Thomas Monjalon
> > > xstats are driver agnostic and have a well-defined naming scheme.
> >
> > Indeed, described here:
> > http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html#extended-
> > statistics-api
> 
> Thanks for sharing.  I do think what is in the link is well thought out but it also may
> not match how the application would choose to represent that stat (capitalization,
> abbreviation, etc).

Welcome. Sure, an application can match any xstats string to its "Display" counterpart,
or a few simple translations actually make (almost) all of them human readable. Translating
an _ to space, capitalize first letter of description items, and remove the unit at the end).
This use-case was considered when detailing the naming scheme.


> > The "rules" of encoding a statistic as an xstats string are pretty simple,
> > and if any PMD breaks the rules, it should be considered broken and fixed.
> 
> I understand it may be broken, but that doesn't help finding them and ensuring they aren't
> broken to begin with.  What regression/automation is in place to ensure drivers aren't
> broken?

Currently nothing automated - patch review on the mailing list. I'm open to ideas on this,
if you have suggestions.


> > Do you see a fundamental problem with parsing the strings to retrieve
> > values?
> 
> I think parsing strings is expensive CPU wise

Parsing the strings can be done once at startup, and the index of the statistics in the
array cached. When actually reading statistics, access of the array of statistics at the
previously cached index will return the same statistic. It is not needed to do strcmp()
per statistic per read.


> That's why I was wondering if a binary id could be generated instead.

I've worked with such a system before, it dynamically registered string->int mappings at
runtime, and allowed "reverse" mapping them too. Its workable, and I'm not opposed to it
if the conclusion is that it really is necessary, but I'm not sure about that.


> The API
> would be very similar to the current xstats API except it would pass id/value pairs
> instead of string/value pairs.  This avoids string comparisons to figure out while still
> allowing flexibility between drivers.

Apart from a once-off scan at startup, xstats achieves this. 


>  It would also 100% guarantee that any strings
> returned by "const char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent across
> devices.

Yes - that is a nice feature that xstats (in its current form) doesn't have.
But what price is there to pay for this?
We need to map an ID for each stat that exists.

How and where will this mapping happen? Perhaps would you expand a little on how
you see this working? 

> I also think there is less chance for error if drivers assign their stats to ID versus
> constructing a string. 

Have a look in how the mapping is done in the xstats implementation for ixgbe for example:
http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n540

It's a pretty simple  {"string stat name" -> offsetof( stuct hw, register_var_name )}


> I haven't checked my application, but I wonder if the binary size
> would actually be smaller if all the stats strings were centrally located versus
> distributed across binaries (highly dependent on the linker and optimization level).

Sounds like optimizing the wrong thing to me.


> > If you like I could code up a minimal sample-app that only pulls
> > statistics, and you can show "interest" in various statistics for printing
> > / monitoring?
> 
> Appreciate the offer.  I actually understand what's is available.  And, BTW, I apologize
> for being late to the game (looks like this was discussed last summer) but I'm just now
> getting looped in and following the mailer but I'm just wondering if something that is
> performance friendly for the user/application and flexible for the drivers is possible.

We're all looking for the same thing - just different approaches that's all :)


RE: Thomas asking about performance numbers:
I can scrape together some raw tsc data on Monday and post to list, and we can discuss it more then.


Regards, -Harry

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[David Harton (dharton)]

> > > Do you see a fundamental problem with parsing the strings to
> > > retrieve values?
> >
> > I think parsing strings is expensive CPU wise
> 
> Parsing the strings can be done once at startup, and the index of the
> statistics in the array cached. When actually reading statistics, access
> of the array of statistics at the previously cached index will return the
> same statistic. It is not needed to do strcmp() per statistic per read.

How is this order guaranteed through the API?  The stat is currently identified by the string not by order returned.  What if a driver returns more/less stats based on config that changes after boot?

> 
> 
> > That's why I was wondering if a binary id could be generated instead.
> 
> I've worked with such a system before, it dynamically registered string-
> >int mappings at runtime, and allowed "reverse" mapping them too. Its
> workable, and I'm not opposed to it if the conclusion is that it really is
> necessary, but I'm not sure about that.
> 
> 
> > The API
> > would be very similar to the current xstats API except it would pass
> > id/value pairs instead of string/value pairs.  This avoids string
> > comparisons to figure out while still allowing flexibility between
> drivers.
> 
> Apart from a once-off scan at startup, xstats achieves this.

Partially true.  You may not need to perform strcmp() per read, although I don't believe the defined API guarantees that this would be safe (see above); but, you still have to perform a strcpy() of each stat whether the caller is interested in it or not.

> 
> 
> >  It would also 100% guarantee that any strings returned by "const
> > char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent across
> > devices.
> 
> Yes - that is a nice feature that xstats (in its current form) doesn't
> have.
> But what price is there to pay for this?
> We need to map an ID for each stat that exists.
> 
> How and where will this mapping happen? Perhaps would you expand a little
> on how you see this working?

I wasn't thinking dynamic registration as that might be more than necessary.  I can code up a detailed snippet that shares the idea if wanted but I think the following communicates the basic idea:

enum rte_eth_stat_e {
    /* accurate desc #1 */
    RTE_ETH_STAT_1, 
    /* accurate desc #2 */
    RTE_ETH_STAT_2,
...
}
struct rte_eth_id_stat {
    rte_eth_stat_e id;
    uin64_t value;
}

int rte_eth_id_stats_num(uint8_t port_id, uint32_t *num_stats);
/* returns < 0 on error or the number of stats that could have been read (i.e. if userd  */
int rte_eth_id_stats_get(uint8_t port_id, uint32_t num_stats, rte_eth_id_stat *id_stats);
const char* rte_eth_id_stat_str(rte_eth_stat_e id);

This allows a driver to return whatever stats that it supports in a consistent manner and also in a performance friendly way.  In fact, the driver side would be identical to what they have today but instead of having arrays with "string stat name" they will have the rte_eth_stat_e.

> 
> > I also think there is less chance for error if drivers assign their
> > stats to ID versus constructing a string.
> 
> Have a look in how the mapping is done in the xstats implementation for
> ixgbe for example:
> http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n540
> 
> It's a pretty simple  {"string stat name" -> offsetof( stuct hw,
> register_var_name )}

It's not how they add the strings but rather the format of the string itself that is error prone.  
IOTW, the "string stat name" is prone to not being formatted correctly or consistently.

> 
> 
> > I haven't checked my application, but I wonder if the binary size
> > would actually be smaller if all the stats strings were centrally
> > located versus distributed across binaries (highly dependent on the
> linker and optimization level).
> 
> Sounds like optimizing the wrong thing to me.

Wasn't trying to optimize image size as much as saying it's a side benefit due to string consolidation.

> 
> 
> > > If you like I could code up a minimal sample-app that only pulls
> > > statistics, and you can show "interest" in various statistics for
> > > printing / monitoring?
> >
> > Appreciate the offer.  I actually understand what's is available.
> > And, BTW, I apologize for being late to the game (looks like this was
> > discussed last summer) but I'm just now getting looped in and
> > following the mailer but I'm just wondering if something that is
> performance friendly for the user/application and flexible for the drivers
> is possible.
> 
> We're all looking for the same thing - just different approaches that's
> all :)
> 
> 
> RE: Thomas asking about performance numbers:
> I can scrape together some raw tsc data on Monday and post to list, and we
> can discuss it more then.

I can do the same if desired.  But, just to make sure we are discussing the same issue:

1) call rte_eth_xtats_get()
This will result in many string copies and depending on the driver *many* copies I don't want or care about.
2) "tokenize"/parse/hash the string returned to identify what the stat actually is
I'm guessing you are stating that this step could be mitigated at startup.  But, again, I don't think the API provides a guarantee which usually leads to bugs over time.
3) Copy the value of the stat into the driver agnostic container the application uses
4) Repeat steps 1-3 for every interface being serviced every 5 or 10 secs

Contrast that to my suggestion which has no string copies and a compile time mapping between "stat_id" and "app stat" can be created for step 2.  I think the performance differences are obvious even without generating cycle times.

I know that dpdk is largely focused on data plane performance and rightly so, but, I'm hoping we can keep the control plane in mind as well especially in the areas around stats or things that happen periodically per port.

Regards,
Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Matthew Hall]

On Fri, Jan 22, 2016 at 06:02:24PM +0300, Igor Ryzhov wrote:
> How about exposing stats according to IF-MIB?
> 
> Statistics to be exposed are - octets, unicast packets, multicast packets, 
> broadcast packets, errors and discards for both TX and RX.
> 
> These counters are basic and implemented by most of drivers.

To be a bit more specific it would be good to have IF-MIB ifTable with the 
items from ifXTable as well:

ifIndex
ifMtu
ifHighSpeed
ifPromiscuousMode
ifPhysAddress
ifConnectorPresent

ifHCInOctets
ifHCInUcastPkts
ifHCInMulticastPkts
ifHCInBroadcastPkts
ifInDiscards
ifInErrors
ifInUnknownProtos

ifHCOutOctets
ifHCOutUcastPkts
ifHCOutMulticastPkts
ifHCOutBroadcastPkts
ifOutDiscards
ifOutErrors

A number of things are missing or weird in the DPDK stats interface. Then I 
get stuck trying to maintain them in my app instead and it's annoying.

Also, it is nice to get the struct populated atomically so the values are as 
self-consistent as possible. If you have to call a function separately on each 
stat it makes them self-inconsistent because it is less atomically populated.

>From long experience, this inconsistency is quite annoying when trying to make 
very accurate traffic measurements in network management software.

Matthew.

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Van Haaren, Harry]

> From: David Harton
> 
> enum rte_eth_stat_e {
>     /* accurate desc #1 */
>     RTE_ETH_STAT_1,
>     /* accurate desc #2 */
>     RTE_ETH_STAT_2,
> ...
> }
> struct rte_eth_id_stat {
>     rte_eth_stat_e id;
>     uin64_t value;
> }
> 
> int rte_eth_id_stats_num(uint8_t port_id, uint32_t *num_stats);
> /* returns < 0 on error or the number of stats that could have been read (i.e. if userd
> */
> int rte_eth_id_stats_get(uint8_t port_id, uint32_t num_stats, rte_eth_id_stat *id_stats);
> const char* rte_eth_id_stat_str(rte_eth_stat_e id);
> 
> This allows a driver to return whatever stats that it supports in a consistent manner and
> also in a performance friendly way.  In fact, the driver side would be identical to what
> they have today but instead of having arrays with "string stat name" they will have the
> rte_eth_stat_e.


Thanks for the code and explanation.


> > RE: Thomas asking about performance numbers:
> > I can scrape together some raw tsc data on Monday and post to list, and we
> > can discuss it more then.
> 
> I can do the same if desired.  But, just to make sure we are discussing the same issue:
> 
> 1) call rte_eth_xtats_get()
> This will result in many string copies and depending on the driver *many* copies I don't
> want or care about.
> 2) "tokenize"/parse/hash the string returned to identify what the stat actually is
> I'm guessing you are stating that this step could be mitigated at startup.  But, again, I
> don't think the API provides a guarantee which usually leads to bugs over time.
> 3) Copy the value of the stat into the driver agnostic container the application uses
> 4) Repeat steps 1-3 for every interface being serviced every 5 or 10 secs
> 
> Contrast that to my suggestion which has no string copies and a compile time mapping
> between "stat_id" and "app stat" can be created for step 2.  I think the performance
> differences are obvious even without generating cycle times.


Indeed using integers will reduce overhead compared to
strings, and the helper function to convert the integer
to string provides the same possibilities as the current
API (in a different way).

I haven't collected performance data yet, apologies for
the delay. Perhaps continuing this conversation after the
V1 patch deadline at the end of the week is a good idea?
I'll have more time to dedicate to thinking about this.


-Harry

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[David Harton (dharton)]

Hi folks,

I didn't see any follow up to this response.

My original concern was rte_eth_stats_get() moving away from a more conventional based definition (note, I believe Matthew Hall made an interesting suggestion to follow a MIB based definition elsewhere).  However, if modifying that API is not desired then I'd really like to have some feedback about extending the current xstats model.

Again, it is desired not to have to copy and/or parse strings for scalability reasons but still maintain the "ABI flexibility" for which the xstats model was geared towards.

Thanks,
Dave


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Harton (dharton)
> Sent: Friday, January 22, 2016 2:26 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()
> 
> > > > Do you see a fundamental problem with parsing the strings to
> > > > retrieve values?
> > >
> > > I think parsing strings is expensive CPU wise
> >
> > Parsing the strings can be done once at startup, and the index of the
> > statistics in the array cached. When actually reading statistics,
> > access of the array of statistics at the previously cached index will
> > return the same statistic. It is not needed to do strcmp() per statistic
> per read.
> 
> How is this order guaranteed through the API?  The stat is currently
> identified by the string not by order returned.  What if a driver returns
> more/less stats based on config that changes after boot?
> 
> >
> >
> > > That's why I was wondering if a binary id could be generated instead.
> >
> > I've worked with such a system before, it dynamically registered
> > string-
> > >int mappings at runtime, and allowed "reverse" mapping them too. Its
> > workable, and I'm not opposed to it if the conclusion is that it
> > really is necessary, but I'm not sure about that.
> >
> >
> > > The API
> > > would be very similar to the current xstats API except it would pass
> > > id/value pairs instead of string/value pairs.  This avoids string
> > > comparisons to figure out while still allowing flexibility between
> > drivers.
> >
> > Apart from a once-off scan at startup, xstats achieves this.
> 
> Partially true.  You may not need to perform strcmp() per read, although I
> don't believe the defined API guarantees that this would be safe (see
> above); but, you still have to perform a strcpy() of each stat whether the
> caller is interested in it or not.
> 
> >
> >
> > >  It would also 100% guarantee that any strings returned by "const
> > > char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent
> > > across devices.
> >
> > Yes - that is a nice feature that xstats (in its current form) doesn't
> > have.
> > But what price is there to pay for this?
> > We need to map an ID for each stat that exists.
> >
> > How and where will this mapping happen? Perhaps would you expand a
> > little on how you see this working?
> 
> I wasn't thinking dynamic registration as that might be more than
> necessary.  I can code up a detailed snippet that shares the idea if
> wanted but I think the following communicates the basic idea:
> 
> enum rte_eth_stat_e {
>     /* accurate desc #1 */
>     RTE_ETH_STAT_1,
>     /* accurate desc #2 */
>     RTE_ETH_STAT_2,
> ...
> }
> struct rte_eth_id_stat {
>     rte_eth_stat_e id;
>     uin64_t value;
> }
> 
> int rte_eth_id_stats_num(uint8_t port_id, uint32_t *num_stats);
> /* returns < 0 on error or the number of stats that could have been read
> (i.e. if userd  */ int rte_eth_id_stats_get(uint8_t port_id, uint32_t
> num_stats, rte_eth_id_stat *id_stats); const char*
> rte_eth_id_stat_str(rte_eth_stat_e id);
> 
> This allows a driver to return whatever stats that it supports in a
> consistent manner and also in a performance friendly way.  In fact, the
> driver side would be identical to what they have today but instead of
> having arrays with "string stat name" they will have the rte_eth_stat_e.
> 
> >
> > > I also think there is less chance for error if drivers assign their
> > > stats to ID versus constructing a string.
> >
> > Have a look in how the mapping is done in the xstats implementation
> > for ixgbe for example:
> > http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n540
> >
> > It's a pretty simple  {"string stat name" -> offsetof( stuct hw,
> > register_var_name )}
> 
> It's not how they add the strings but rather the format of the string
> itself that is error prone.
> IOTW, the "string stat name" is prone to not being formatted correctly or
> consistently.
> 
> >
> >
> > > I haven't checked my application, but I wonder if the binary size
> > > would actually be smaller if all the stats strings were centrally
> > > located versus distributed across binaries (highly dependent on the
> > linker and optimization level).
> >
> > Sounds like optimizing the wrong thing to me.
> 
> Wasn't trying to optimize image size as much as saying it's a side benefit
> due to string consolidation.
> 
> >
> >
> > > > If you like I could code up a minimal sample-app that only pulls
> > > > statistics, and you can show "interest" in various statistics for
> > > > printing / monitoring?
> > >
> > > Appreciate the offer.  I actually understand what's is available.
> > > And, BTW, I apologize for being late to the game (looks like this
> > > was discussed last summer) but I'm just now getting looped in and
> > > following the mailer but I'm just wondering if something that is
> > performance friendly for the user/application and flexible for the
> > drivers is possible.
> >
> > We're all looking for the same thing - just different approaches
> > that's all :)
> >
> >
> > RE: Thomas asking about performance numbers:
> > I can scrape together some raw tsc data on Monday and post to list,
> > and we can discuss it more then.
> 
> I can do the same if desired.  But, just to make sure we are discussing
> the same issue:
> 
> 1) call rte_eth_xtats_get()
> This will result in many string copies and depending on the driver *many*
> copies I don't want or care about.
> 2) "tokenize"/parse/hash the string returned to identify what the stat
> actually is I'm guessing you are stating that this step could be mitigated
> at startup.  But, again, I don't think the API provides a guarantee which
> usually leads to bugs over time.
> 3) Copy the value of the stat into the driver agnostic container the
> application uses
> 4) Repeat steps 1-3 for every interface being serviced every 5 or 10 secs
> 
> Contrast that to my suggestion which has no string copies and a compile
> time mapping between "stat_id" and "app stat" can be created for step 2.
> I think the performance differences are obvious even without generating
> cycle times.
> 
> I know that dpdk is largely focused on data plane performance and rightly
> so, but, I'm hoping we can keep the control plane in mind as well
> especially in the areas around stats or things that happen periodically
> per port.
> 
> Regards,
> Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Matthew Hall]

On Mon, Feb 01, 2016 at 04:47:56PM +0000, David Harton (dharton) wrote:
> Hi folks,
> 
> I didn't see any follow up to this response.
> 
> My original concern was rte_eth_stats_get() moving away from a more 
> conventional based definition (note, I believe Matthew Hall made an 
> interesting suggestion to follow a MIB based definition elsewhere).  
> However, if modifying that API is not desired then I'd really like to have 
> some feedback about extending the current xstats model.
> 
> Again, it is desired not to have to copy and/or parse strings for 
> scalability reasons but still maintain the "ABI flexibility" for which the 
> xstats model was geared towards.
> 
> Thanks,
> Dave

For me, I'd like to be able to get the core common stats in single memory 
blocks which are as self-consistent and atomic as possible.

I'd prefer to only resort to stuff like xstats using some kind of 
string-to-number lookup at the beginning, and only for weird stuff not the 
common MIB-like items.

>From past experience this would be very valuable.

Matthew.

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Van Haaren, Harry]

+John,

> From: David Harton 
> Subject: RE: [dpdk-dev] Future Direction for rte_eth_stats_get()
> 
> Hi folks,
> 
> I didn't see any follow up to this response.

I think you may have missed one:
http://dpdk.org/ml/archives/dev/2016-January/032211.html


> <snip>
> However, if modifying that API is not desired then I'd
> really like to have some feedback about extending the current xstats model.

API / ABI modification/breakage is something that will
need to be considered in any discussions. The 2.3 (or 16.04)
V1 patch deadline has passed, so any output from this 
discussion will be leading to the 16.07 release.

> Again, it is desired not to have to copy and/or parse strings for scalability reasons but
> still maintain the "ABI flexibility" for which the xstats model was geared towards.

Agreed.


From: David Harton
> enum rte_eth_stat_e {
>     /* accurate desc #1 */
>     RTE_ETH_STAT_1,
>     /* accurate desc #2 */
>     RTE_ETH_STAT_2,
> ...
> }

For this enum, do you see each stat for every PMD entered here?
RX_GOOD_PACKETS,

What about RX and TX, is it listed twice?
RX_GOOD_PACKETS,
TX_GOOD_PACKETS,

In a case where a VF has RX_GOOD_PACKETS, does it get its "own" set?
RX_GOOD_PACKETS,
TX_GOOD_PACKETS,
VF_RX_GOOD_PACKETS,
VF_TX_GOOD_PACKETS,

I'm looking at the enum thinking it will grow out of control.
Have you thought about adding metadata for RX / TX, PF / VF?

If metadata info is added, it would make retrieving a set of
statistics based on a certain mask much easier. Do you think
this may be of use?


-Harry 

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Tahhan, Maryam]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Matthew Hall
> Sent: Friday, January 22, 2016 8:49 PM
> To: Igor Ryzhov <iryzhov@nfware.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()
> 
> On Fri, Jan 22, 2016 at 06:02:24PM +0300, Igor Ryzhov wrote:
> > How about exposing stats according to IF-MIB?
> >
> > Statistics to be exposed are - octets, unicast packets, multicast
> > packets, broadcast packets, errors and discards for both TX and RX.
> >
> > These counters are basic and implemented by most of drivers.
> 
> To be a bit more specific it would be good to have IF-MIB ifTable with
> the items from ifXTable as well:
> 

I think the MIBs ifTable would be good for the high level stats across all the drivers for sure, we would need to take backward compatibility for the current stats into account. 

> ifIndex
> ifMtu
> ifHighSpeed
> ifPromiscuousMode
> ifPhysAddress
> ifConnectorPresent
> 
> ifHCInOctets
> ifHCInUcastPkts
> ifHCInMulticastPkts
> ifHCInBroadcastPkts
> ifInDiscards
> ifInErrors
> ifInUnknownProtos
> 
> ifHCOutOctets
> ifHCOutUcastPkts
> ifHCOutMulticastPkts
> ifHCOutBroadcastPkts
> ifOutDiscards
> ifOutErrors
> 
> A number of things are missing or weird in the DPDK stats interface.
> Then I get stuck trying to maintain them in my app instead and it's
> annoying.
> 
> Also, it is nice to get the struct populated atomically so the values are as
> self-consistent as possible. If you have to call a function separately on
> each stat it makes them self-inconsistent because it is less atomically
> populated.

+1
> 
> From long experience, this inconsistency is quite annoying when trying to
> make very accurate traffic measurements in network management
> software.
> 
> Matthew.

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Kyle Larose]

On Tue, Feb 2, 2016 at 7:44 AM, Tahhan, Maryam <maryam.tahhan@intel.com> wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Matthew Hall
>> Sent: Friday, January 22, 2016 8:49 PM
>> To: Igor Ryzhov <iryzhov@nfware.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()
>>
>> On Fri, Jan 22, 2016 at 06:02:24PM +0300, Igor Ryzhov wrote:
>> > How about exposing stats according to IF-MIB?
>> >
>> > Statistics to be exposed are - octets, unicast packets, multicast
>> > packets, broadcast packets, errors and discards for both TX and RX.
>> >
>> > These counters are basic and implemented by most of drivers.
>>
>> To be a bit more specific it would be good to have IF-MIB ifTable with
>> the items from ifXTable as well:
>>
>
> I think the MIBs ifTable would be good for the high level stats across all the drivers for sure, we would need to take backward compatibility for the current stats into account.
>
>> ifIndex
>> ifMtu
>> ifHighSpeed
>> ifPromiscuousMode
>> ifPhysAddress
>> ifConnectorPresent
>>
>> ifHCInOctets
>> ifHCInUcastPkts
>> ifHCInMulticastPkts
>> ifHCInBroadcastPkts
>> ifInDiscards
>> ifInErrors
>> ifInUnknownProtos
>>
>> ifHCOutOctets
>> ifHCOutUcastPkts
>> ifHCOutMulticastPkts
>> ifHCOutBroadcastPkts
>> ifOutDiscards
>> ifOutErrors
>>
>> A number of things are missing or weird in the DPDK stats interface.
>> Then I get stuck trying to maintain them in my app instead and it's
>> annoying.
>>
>> Also, it is nice to get the struct populated atomically so the values are as
>> self-consistent as possible. If you have to call a function separately on
>> each stat it makes them self-inconsistent because it is less atomically
>> populated.
>
> +1

I also agree about the ifTable/ifXTable. I think that a few other
ethernet oriented MIBs may also be worth considering. The RMON MIB's
etherStatsTable has some useful counters in it (namely, the packet
size histogram counters). We could also do the dot3StatsTable from the
EtherLike MIB, though I'm not sure how useful it would be.

>>
>> From long experience, this inconsistency is quite annoying when trying to
>> make very accurate traffic measurements in network management
>> software.
>>
>> Matthew.

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[David Harton (dharton)]

> From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> 
> > From: David Harton
> > Subject: RE: [dpdk-dev] Future Direction for rte_eth_stats_get()
> >
> > Hi folks,
> >
> > I didn't see any follow up to this response.
> 
> I think you may have missed one:
> http://dpdk.org/ml/archives/dev/2016-January/032211.html

Apologies Harry!  I didn't see your original post because the IT gods had decided your response was "Junk" mail and it didn't make it to my dev_dpdk.org mail folder. :(

A colleague actually pointed me to this post separately today.  I've made the Junk mailer a little smarter now...hopefully.

<snip>
> 
> I'm looking at the enum thinking it will grow out of control.
> Have you thought about adding metadata for RX / TX, PF / VF?

Yes, after thinking about it more I think it could get crazy.

> 
> If metadata info is added, it would make retrieving a set of statistics
> based on a certain mask much easier. Do you think this may be of use?

Actually, I put a fair bit of thought into things and then realized, why re-invent the wheel?
Why not follow the ethtool stats model?

struct rte_eth_xstats_name {
    char name[RTE_ETH_XSTATS_NAME_SIZE]; };

extern int rte_eth_xtats_count(uint8_t port_id, unsigned *count);
extern int rte_eth_xtats_strings(uint8_t port_id, unsigned count, struct rte_eth_xtats_name *names);
extern int rte_eth_xtats_values(uint8_t port_id, unsigned count, uint64_t *values);

The existing API could be left in-place and these could be added for folks that don't want to grab the strings all the time.

The cons compared to providing an enum or extending struct rte_eth_stats are:
 - you have to perform a query immediately after the device is attached
 - doesn't require conformity...which has pros and cons

I'm actually testing the changes above if folks think this would be a reasonable compromise I can patch them up.

I still feel the feedback myself and others gave about rte_eth_stats_get() being closer to a standard MIB should get some consideration.  Applications that run with a number of different drivers/device types likely want to avoid having to create "xstats mapping tables" every time a new device pops out just so they can debug problems.

Thanks,
Dave

Re: [dpdk-dev] Future Direction for rte_eth_stats_get()

[Tahhan, Maryam]

> From: David Harton (dharton) [mailto:dharton@cisco.com]
> Sent: Friday, February 5, 2016 9:16 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Thomas
> Monjalon <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; Tahhan, Maryam <maryam.tahhan@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>
> Subject: RE: [dpdk-dev] Future Direction for rte_eth_stats_get()
> 
> 
> > From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> >
> > > From: David Harton
> > > Subject: RE: [dpdk-dev] Future Direction for rte_eth_stats_get()
> > >
> > > Hi folks,
> > >
> > > I didn't see any follow up to this response.
> >
> > I think you may have missed one:
> > http://dpdk.org/ml/archives/dev/2016-January/032211.html
> 
> Apologies Harry!  I didn't see your original post because the IT gods had
> decided your response was "Junk" mail and it didn't make it to my
> dev_dpdk.org mail folder. :(
> 
> A colleague actually pointed me to this post separately today.  I've made
> the Junk mailer a little smarter now...hopefully.
> 
> <snip>
> >
> > I'm looking at the enum thinking it will grow out of control.
> > Have you thought about adding metadata for RX / TX, PF / VF?
> 
> Yes, after thinking about it more I think it could get crazy.
> 
> >
> > If metadata info is added, it would make retrieving a set of
> > statistics based on a certain mask much easier. Do you think this may
> be of use?
> 
> Actually, I put a fair bit of thought into things and then realized, why re-
> invent the wheel?
> Why not follow the ethtool stats model?
> 
> struct rte_eth_xstats_name {
>     char name[RTE_ETH_XSTATS_NAME_SIZE]; };
> 
> extern int rte_eth_xtats_count(uint8_t port_id, unsigned *count); extern
> int rte_eth_xtats_strings(uint8_t port_id, unsigned count, struct
> rte_eth_xtats_name *names); extern int rte_eth_xtats_values(uint8_t
> port_id, unsigned count, uint64_t *values);
> 
> The existing API could be left in-place and these could be added for folks
> that don't want to grab the strings all the time.
> 
> The cons compared to providing an enum or extending struct
> rte_eth_stats are:
>  - you have to perform a query immediately after the device is attached
>  - doesn't require conformity...which has pros and cons
> 
> I'm actually testing the changes above if folks think this would be a
> reasonable compromise I can patch them up.
> 

I think this is a reasonable compromise. 

> I still feel the feedback myself and others gave about rte_eth_stats_get()
> being closer to a standard MIB should get some consideration.
 +1

> Applications that run with a number of different drivers/device types
> likely want to avoid having to create "xstats mapping tables" every time
> a new device pops out just so they can debug problems.
> 
> Thanks,
> Dave