| Message ID | 1472206104-22035-1-git-send-email-wei.dai@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Thomas Monjalon |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 46B8858D8; Fri, 26 Aug 2016 12:09:10 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 5A9672BC6 for <dev@dpdk.org>; Fri, 26 Aug 2016 12:09:09 +0200 (CEST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga104.fm.intel.com with ESMTP; 26 Aug 2016 03:09:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,580,1464678000"; d="scan'208";a="1453279" Received: from dpdk4.bj.intel.com ([172.16.182.166]) by orsmga005.jf.intel.com with ESMTP; 26 Aug 2016 03:09:07 -0700 From: Wei Dai <wei.dai@intel.com> To: dev@dpdk.org Cc: Wei Dai <wei.dai@intel.com> Date: Fri, 26 Aug 2016 18:08:24 +0800 Message-Id: <1472206104-22035-1-git-send-email-wei.dai@intel.com> X-Mailer: git-send-email 2.7.4 Subject: [dpdk-dev] [PATCH] ethdev: fix statistics description X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Wei Dai
Aug. 26, 2016, 10:08 a.m. UTC
Add comments to describe that not all statistics fields in
struct rte_eth_stats are supported by any type of network
interface card. If any statistics field is not supported,
its value is 0.
Fixes: af75078fece3 ("first public release")
Signed-off-by: Wei Dai <wei.dai@intel.com>
---
lib/librte_ether/rte_ethdev.h | 3 +++
1 file changed, 3 insertions(+)
Comments
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?
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. 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?
> -----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
> -----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
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
> -----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
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).
> > 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? >
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index b0fe033..653e36c 100644 --- a/lib/librte_ether/rte_ethdev.h +++ b/lib/librte_ether/rte_ethdev.h @@ -190,6 +190,9 @@ struct rte_mbuf; /** * 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 { uint64_t ipackets; /**< Total number of successfully received packets. */