* Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
2016-11-02 8:28 ` Dai, Wei
@ 2016-11-02 9:07 ` Mcnamara, John
2016-11-03 2:00 ` Remy Horton
2016-11-02 9:21 ` Morten Brørup
2016-11-08 13:33 ` Tahhan, Maryam
2 siblings, 1 reply; 9+ messages in thread
From: Mcnamara, John @ 2016-11-02 9:07 UTC (permalink / raw)
To: Dai, Wei, Thomas Monjalon, Ananyev, Konstantin, Wu, Jingjing,
Zhang, Helin, Curran, Greg, Horton, Remy, Van Haaren, Harry
Cc: dev
> -----Original Message-----
> From: Dai, Wei
> Sent: Wednesday, November 2, 2016 8:29 AM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Zhang, Helin <helin.zhang@intel.com>; Dai, Wei <wei.dai@intel.com>;
> Curran, Greg <greg.curran@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] ethdev: fix statistics description
>
> Hi, John & Greg
>
> Would you please give any opinion for this patch ?
>
> I have looked through all PMDs and found not all statistics items can be
> supported by some NIC.
> For example, rx_nombuf, q_ipackets, q_opackets, q_ibytes and q_obytes
> are not supported by i40e.
> But when the function rte_eth_stats_get(uint8_t port_id, struct
> rte_eth_stats *stats) is called for i40e PMD, Above un-supported
> statistics item in output stats are zero, this is not real value.
> So far, there is no way to know whether an item in struct rte_eth_stats is
> supported or not only from this structure definition.
> Maybe some structure member can be added to indicate each of statistics
> item valid or not.
> But this means ABI change.
>
> In following list, I list statistics support details of all PMDs.
> Hope it can be displayed in your screen.
Hi,
Thanks for the analysis.
Perhaps we could an API that returns a struct, or otherwise, that indicated what stats are returned by a PMD. An application that required stats could call it once to establish what stats were available. It would have to be done in some way that wouldn't break ABI every time a new stat was added.
Harry, Remy, how would this fit in with the existing stats scheme or the new metrics library.
John
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
2016-11-02 9:07 ` Mcnamara, John
@ 2016-11-03 2:00 ` Remy Horton
2016-11-03 9:07 ` Morten Brørup
0 siblings, 1 reply; 9+ messages in thread
From: Remy Horton @ 2016-11-03 2:00 UTC (permalink / raw)
To: Mcnamara, John, Dai, Wei, Thomas Monjalon, Ananyev, Konstantin,
Wu, Jingjing, Zhang, Helin, Curran, Greg, Van Haaren, Harry
Cc: dev
On 02/11/2016 17:07, Mcnamara, John wrote:
[..]
> Perhaps we could an API that returns a struct, or otherwise, that
> indicated what stats are returned by a PMD. An application that
> required stats could call it once to establish what stats were
> available. It would have to be done in some way that wouldn't break
> ABI every time a new stat was added.
>
> Harry, Remy, how would this fit in with the existing stats scheme or
> the new metrics library.
At the moment xstats (rte_eth_xstats_get()) pulls stuff out of
rte_eth_stats and reports them unconditionally alongside all the
driver-specific xstats. This could change so that it only reports the
(legacy) stats the PMDs actually fills in.
Personally in the longer term I think xstats should get all the info it
requires directly rather than relying on the legacy stats for some of
its info, but that would involve pushing a lot of common code into the
PMDs..
..Remy
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
2016-11-03 2:00 ` Remy Horton
@ 2016-11-03 9:07 ` Morten Brørup
0 siblings, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2016-11-03 9:07 UTC (permalink / raw)
To: Remy Horton, Mcnamara, John, Dai, Wei, Thomas Monjalon, Ananyev,
Konstantin, Wu, Jingjing, Zhang, Helin, Curran, Greg, Van Haaren,
Harry
Cc: dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Thursday, November 3, 2016 3:01 AM
>
> On 02/11/2016 17:07, Mcnamara, John wrote:
> [..]
> > Perhaps we could an API that returns a struct, or otherwise, that
> > indicated what stats are returned by a PMD. An application that
> > required stats could call it once to establish what stats were
> > available. It would have to be done in some way that wouldn't break
> > ABI every time a new stat was added.
> >
> > Harry, Remy, how would this fit in with the existing stats scheme or
> > the new metrics library.
>
> At the moment xstats (rte_eth_xstats_get()) pulls stuff out of
> rte_eth_stats and reports them unconditionally alongside all the
> driver-specific xstats. This could change so that it only reports the
> (legacy) stats the PMDs actually fills in.
>
> Personally in the longer term I think xstats should get all the info it
> requires directly rather than relying on the legacy stats for some of
> its info, but that would involve pushing a lot of common code into the
> PMDs..
>
> ..Remy
Adding eth_stats to eth_xstats or not is not important - it's not a synchronized snapshot of the entire counter set, just a question of calling one or two functions to obtain the values.
Regarding eth_xstats, I would dare to say that the NIC HW designers chose their statistics counters wisely, based on a combination of industry standards (e.g. common SNMP MIBs, such as the Interfaces MIB and etherStats) and customer feedback, so the hardware counters are probably useful to a DPDK application, and thus it makes sense to expose them directly. The application can transform them into industry standard counter sets (IF-MIB, etherStats, etc.) if required. DPDK could offer a common library for this transformation.
-Morten
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
2016-11-02 8:28 ` Dai, Wei
2016-11-02 9:07 ` Mcnamara, John
@ 2016-11-02 9:21 ` Morten Brørup
2016-11-07 20:37 ` Thomas Monjalon
2016-11-08 13:33 ` Tahhan, Maryam
2 siblings, 1 reply; 9+ messages in thread
From: Morten Brørup @ 2016-11-02 9:21 UTC (permalink / raw)
To: Dai, Wei, Thomas Monjalon, Mcnamara, John, Ananyev, Konstantin,
Wu, Jingjing, Zhang, Helin, Curran, Greg
Cc: dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dai, Wei
> Sent: Wednesday, November 2, 2016 9:29 AM
> To: Thomas Monjalon; Mcnamara, John; Ananyev, Konstantin; Wu, Jingjing;
> Zhang, Helin; Dai, Wei; Curran, Greg
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
>
> Hi, John & Greg
>
> Would you please give any opinion for this patch ?
>
> I have looked through all PMDs and found not all statistics items can
> be supported by some NIC.
> For example, rx_nombuf, q_ipackets, q_opackets, q_ibytes and
> q_obytes are not supported by i40e.
> But when the function rte_eth_stats_get(uint8_t port_id, struct
> rte_eth_stats *stats) is called for i40e PMD, Above un-supported
> statistics item in output stats are zero, this is not real value.
> So far, there is no way to know whether an item in struct rte_eth_stats
> is supported or not only from this structure definition.
> Maybe some structure member can be added to indicate each of statistics
> item valid or not.
> But this means ABI change.
>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Tuesday, October 4, 2016 5:35 PM
> > To: Dai, Wei <wei.dai@intel.com>; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
> >
> > 2016-08-26 18:08, Wei Dai:
> > > /**
> > > * A structure used to retrieve statistics for an Ethernet port.
> > > + * Not all statistics fields in struct rte_eth_stats are supported
> > > + * by any type of network interface card (NIC). If any statistics
> > > + * field is not supported, its value is 0 .
> > > */
> > > struct rte_eth_stats {
> >
> > I'm missing the point of this patch.
> > Why do you think it is a fix?
> >
> > John, any opinion?
>
I think the source code comment is an improvement. I would also like to see a source code comment describing the criteria for choosing which counters go in the eth_stats, and which counters are relegated to the eth_xstats.
It doesn't look like the Interfaces MIB (IF-MIB) or even the etherStats MIB drives this selection.
The ifOutQLen in the IF-MIB is deprecated; I guess it is not important for network monitoring. But fast access to the queue lengths are obviously useful for a fast path application with RED or other intelligent queue overflow handling mechanisms, so in my mind they are useful here.
-Morten
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
2016-11-02 9:21 ` Morten Brørup
@ 2016-11-07 20:37 ` Thomas Monjalon
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2016-11-07 20:37 UTC (permalink / raw)
To: Dai, Wei
Cc: Morten Brørup, Mcnamara, John, Ananyev, Konstantin, Wu,
Jingjing, Zhang, Helin, Curran, Greg, dev
2016-11-02 10:21, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-08-26 18:08, Wei Dai:
> > > > /**
> > > > * A structure used to retrieve statistics for an Ethernet port.
> > > > + * Not all statistics fields in struct rte_eth_stats are supported
> > > > + * by any type of network interface card (NIC). If any statistics
> > > > + * field is not supported, its value is 0 .
> > > > */
> > > > struct rte_eth_stats {
> > >
> > > I'm missing the point of this patch.
> > > Why do you think it is a fix?
> > >
> > > John, any opinion?
>
> I think the source code comment is an improvement.
[...]
Applied as an improvement (not a fix).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
2016-11-02 8:28 ` Dai, Wei
2016-11-02 9:07 ` Mcnamara, John
2016-11-02 9:21 ` Morten Brørup
@ 2016-11-08 13:33 ` Tahhan, Maryam
2 siblings, 0 replies; 9+ messages in thread
From: Tahhan, Maryam @ 2016-11-08 13:33 UTC (permalink / raw)
To: Dai, Wei, Thomas Monjalon, Mcnamara, John, Ananyev, Konstantin,
Wu, Jingjing, Zhang, Helin, Dai, Wei, Curran, Greg
Cc: dev
>
> Hi, John & Greg
>
> Would you please give any opinion for this patch ?
>
> I have looked through all PMDs and found not all statistics items can be
> supported by some NIC.
> For example, rx_nombuf, q_ipackets, q_opackets, q_ibytes and q_obytes
> are not supported by i40e.
Queue stats should be supported by i40e as we have access to struct i40e_queue_stats this is a gap. Same for e1000.
For me (from a stats perspective), we should be able to report everything that ethtool can report for the different kernel network drivers (as we have the same base driver code in DPDK). In other words, the DPDK stats API should provide the same set of stats as a standard networking interface would to an external monitoring tool in case we want to perform some sort of analytics on it afterwards.
At a very minimum the top level stats should include: ipackets, opackets, ibytes, obytes, imissed, ierrors, oerrors. The queue stats in theory could be migrated to the xstats, it would require a lot of clean up in existing drivers which is why we didn't remove them when we did the original cleanup of the struct for the xstats API.
> But when the function rte_eth_stats_get(uint8_t port_id, struct
> rte_eth_stats *stats) is called for i40e PMD, Above un-supported statistics
> item in output stats are zero, this is not real value.
Agreed - should not output 0 for these. But should ensure where stats are possible to obtain, we support them in DPDK.
> So far, there is no way to know whether an item in struct rte_eth_stats is
> supported or not only from this structure definition.
> Maybe some structure member can be added to indicate each of statistics
> item valid or not.
> But this means ABI change.
Migrating the queue/nonstandard stats to the xstats API would fix this, the only issue is with the existing drivers that are unsupported fields with 0.
>
> In following list, I list statistics support details of all PMDs.
> Hope it can be displayed in your screen.
>
Thanks for this, it's very helpful. I'm currently collating a list of the missing stats for e1000, ixgbe and i40e from DPDK. So this is very helpful.
> Thanks
> /Wei
>
> NIC ipackets opackets ibytes obytes imissed ierrors oerrors
> rx_nombuf q_ipackets q_opacktes q_ibytes q_obytes q_errors
> af_packet y y y y n n y n y y y y
> y
> bnx2x y y y y y y y y n n n n
> n
> bnxt y y y y y y y n y y y y
> y
> bonding y y y y y y y y y y y y
> y
> cxgbe y y y y y y y n y y y y
> y
> e1000(igb) y y y y y y y n n n n n
> n
> e1000(igbvf) y y y y n n n n n n n
> n n
> ena y y y y y y y y n n n n
> n
> enic y y y y y y y y n n n n
> n
> fm10k y y y y n n n n y y y y
> n
> i40e y y y y y y y n n n n n
> n
> i40evf y y y y n y y n n n n n
> n
> ixgbe y y y y y y y n y y y y
> y
> ixgbevf y y y y n n n n n n n n
> n
> mlx4 y y y y n y y y y y y y
> y
> mlx5 y y y y n y y y y y y y
> y
> mpipe y y y y n y y y y y y y
> y
> nfp y y y y y y y y y y y y
> n
> null y y n n n n y n y y n n
> y
> pcap y y y y n n y n y y y y
> y
> qede y y y y y y y y n n n n
> n
> ring y y n n n n y n y y n n
> y
> szedata2 y y y y n n y n y y y y
> y
> thunderx y y y y y y y n y y y y
> n
> vhost y y y y n n y n y y y y
> n
> virtio y y y y n y y y y y y y
> n
> vmxnet3 y y y y n y y y y y y y
> y
> xenvirt y y n n n n n n n n n n
> n
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Tuesday, October 4, 2016 5:35 PM
> > To: Dai, Wei <wei.dai@intel.com>; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: fix statistics description
> >
> > 2016-08-26 18:08, Wei Dai:
> > > /**
> > > * A structure used to retrieve statistics for an Ethernet port.
> > > + * Not all statistics fields in struct rte_eth_stats are supported
> > > + * by any type of network interface card (NIC). If any statistics
> > > + * field is not supported, its value is 0 .
> > > */
> > > struct rte_eth_stats {
> >
> > I'm missing the point of this patch.
> > Why do you think it is a fix?
> >
> > John, any opinion?
>
^ permalink raw reply [flat|nested] 9+ messages in thread