[dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
Extend rte_eth_xstats_get to retrieve additional stats from the device driver as well the top level extended stats. Add additional drop counters to the extended stats. Signed-off-by: Maryam Tahhan --- lib/librte_ether/rte_ethdev.c | 12 lib/librte_ether/rte_ethdev.h | 4 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index 5a94654..8c22cda 100644 --- a/lib/librte_ether/rte_ethdev.c +++ b/lib/librte_ether/rte_ethdev.c @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] = { {"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)}, {"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)}, {"rx_errors", offsetof(struct rte_eth_stats, ierrors)}, + {"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)}, + {"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)}, {"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)}, {"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)}, {"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)}, @@ -136,6 +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] = { {"rx_flow_control_xon", offsetof(struct rte_eth_stats, rx_pause_xon)}, {"tx_flow_control_xoff", offsetof(struct rte_eth_stats, tx_pause_xoff)}, {"rx_flow_control_xoff", offsetof(struct rte_eth_stats, rx_pause_xoff)}, + {"tx_drops", offsetof(struct rte_eth_stats, odrop)}, + {"rx_drops", offsetof(struct rte_eth_stats, idrop)}, }; #define RTE_NB_STATS (sizeof(rte_stats_strings) / sizeof(rte_stats_strings[0])) @@ -154,7 +158,6 @@ static const struct rte_eth_xstats_name_off rte_txq_stats_strings[] = { #define RTE_NB_TXQ_STATS (sizeof(rte_txq_stats_strings) / \ sizeof(rte_txq_stats_strings[0])) - /** * The user application callback description. * @@ -1741,7 +1744,7 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats, { struct rte_eth_stats eth_stats; struct rte_eth_dev *dev; - unsigned count, i, q; + unsigned count = 0, xcount = 0, i, q; uint64_t val; char *stats_ptr; @@ -1754,18 +1757,19 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats, /* implemented by the driver */ if (dev->dev_ops->xstats_get != NULL) - return (*dev->dev_ops->xstats_get)(dev, xstats, n); + xcount = (*dev->dev_ops->xstats_get)(dev, xstats, n); /* else, return generic statistics */ count = RTE_NB_STATS; count += dev->data->nb_rx_queues * RTE_NB_RXQ_STATS; count += dev->data->nb_tx_queues * RTE_NB_TXQ_STATS; + count += xcount; if (n < count) return count; /* now fill the xstats structure */ - count = 0; + count = xcount; rte_eth_stats_get(port_id, ð_stats); /* global stats */ diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index 16dbe00..5bc3b81 100644 --- a/lib/librte_ether/rte_ethdev.h +++ b/lib/librte_ether/rte_ethdev.h @@ -224,6 +224,10 @@ struct rte_eth_stats { /**< Total number of good bytes received from loopback,VF Only */ uint64_t olbbytes; /**< Total number of good bytes transmitted to loopback,VF Only */ + uint64_t imacerr; /**< Total of RX packets with MAC Errors. */ + uint64_t iphyerr; /**< Total of RX packets with PHY Errors. */ + uint64_t idrop; /**< Total number of dropped received packets. */ + uint64_t odrop; /**< Total number of dropped transmitted packets. */ }; /** -- 1.8.1.4
[dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
2015-06-05 18:35, Maryam Tahhan: > Extend rte_eth_xstats_get to retrieve additional stats from the device > driver as well the top level extended stats. Add additional drop > counters to the extended stats. > > Signed-off-by: Maryam Tahhan [..] Patch 1/4 doesn't compile without patch 2/4. > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off > rte_stats_strings[] = { > {"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)}, > {"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)}, > {"rx_errors", offsetof(struct rte_eth_stats, ierrors)}, > + {"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)}, > + {"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)}, > {"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)}, > {"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)}, > {"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)}, > @@ -136,6 +138,8 @@ static const struct rte_eth_xstats_name_off > rte_stats_strings[] = { > {"rx_flow_control_xon", offsetof(struct rte_eth_stats, rx_pause_xon)}, > {"tx_flow_control_xoff", offsetof(struct rte_eth_stats, tx_pause_xoff)}, > {"rx_flow_control_xoff", offsetof(struct rte_eth_stats, rx_pause_xoff)}, > + {"tx_drops", offsetof(struct rte_eth_stats, odrop)}, > + {"rx_drops", offsetof(struct rte_eth_stats, idrop)}, > }; [...] > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -224,6 +224,10 @@ struct rte_eth_stats { > > /**< Total number of good bytes received from loopback,VF Only */ > uint64_t olbbytes; > /**< Total number of good bytes transmitted to loopback,VF Only */ > > + uint64_t imacerr; /**< Total of RX packets with MAC Errors. */ > + uint64_t iphyerr; /**< Total of RX packets with PHY Errors. */ > + uint64_t idrop; /**< Total number of dropped received packets. */ > + uint64_t odrop; /**< Total number of dropped transmitted packets. */ > }; You are extending the generic stats. This is not the idea behind xstats. The xstats are specific to the driver. Furthermore we should migrate some "not really generic stats" to xstats in order to keep only the really basic and common stats in rte_eth_stats. By the way, in order to avoid duplicated code when getting generic stats through xstats API, we need to change the implementation of rte_eth_xstats_get() to add generic stats automatically, even if the driver provide some xstats.
[dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
On Wed, Jun 17, 2015 at 9:58 AM, Thomas Monjalon wrote: [...] > > You are extending the generic stats. This is not the idea behind xstats. > The xstats are specific to the driver. > Furthermore we should migrate some "not really generic stats" to xstats > in order to keep only the really basic and common stats in rte_eth_stats. > By the way, in order to avoid duplicated code when getting generic stats > through xstats API, we need to change the implementation of > rte_eth_xstats_get() to add generic stats automatically, even if the > driver provide some xstats. This may be the wrong thread for this, and perhaps this question has been answered (I couldn't find anything relating to it), but I have a related question regarding exposing stats that are not captured in the current device independent api (rte_eth_stats_get). Is there a recommended strategy for displaying MIB interface stats for applications using DPDK? For example, consider the IF-MIB (http://www.ietf.org/rfc/rfc2233.txt). It provides three "normal" in packet counters: ifHCInUcastPkts ifHCInMulticastPkts ifHCInBroadcastPkts Looking at rte_eth_stats, the structure returned by rte_eth_stats_get, we can only retrieve the total number of in packets, and the total number of in multicast packets. We can't retrieve the number of in broadcast packets. This means that we neither display ifHCInUcastPkts (which needs us to subtract mulicast and broadcast from the total), nor ifHCInBroadcastPkts. While I understand that some devices may not support classifying packets into such categories, I would think that most NICs would try to allow applications to conform the fairly established standards such as the IF-MIB. One would think that maybe xstats would allow this: these nics have low level stats that provide this information. However, consumers of the API would need to understand each NIC's implementation of the xstats API to use it, making it somewhat cumbersome to use. I'd think that it makes sense for the MIB information to live in a device-independent, well-defined API. Does it make sense to provide sufficient information in the eth_stats_get function to provide support for the various interface stats mibs? Alternatively, does it make sense to make a new function or API tailored to providing this support? It could be device independent (unlike xstats), while allowing each driver to choose whether or not it supports it. I'm using a fairly old version of DPDK (1.7.1), but from looking at the master branch (http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.h), it looks like the structure hasn't really changed. I've been struggling to figure out the right away to approach this, so any thoughts/insights would be much appreciated. Thanks, Kyle
[dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
> 2015-06-05 18:35, Maryam Tahhan: > > Extend rte_eth_xstats_get to retrieve additional stats from the device > > driver as well the top level extended stats. Add additional drop > > counters to the extended stats. > > > > Signed-off-by: Maryam Tahhan > [..] > Patch 1/4 doesn't compile without patch 2/4. The rebased patches should fix this issue. > > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off > rte_stats_strings[] = { > > {"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)}, > > {"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)}, > > {"rx_errors", offsetof(struct rte_eth_stats, ierrors)}, > > + {"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)}, > > + {"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)}, > > {"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)}, > > {"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)}, > > {"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)}, @@ -136,6 > > +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] > = { > > {"rx_flow_control_xon", offsetof(struct rte_eth_stats, > rx_pause_xon)}, > > {"tx_flow_control_xoff", offsetof(struct rte_eth_stats, > tx_pause_xoff)}, > > {"rx_flow_control_xoff", offsetof(struct rte_eth_stats, > > rx_pause_xoff)}, > > + {"tx_drops", offsetof(struct rte_eth_stats, odrop)}, > > + {"rx_drops", offsetof(struct rte_eth_stats, idrop)}, > > }; > [...] > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > @@ -224,6 +224,10 @@ struct rte_eth_stats { > > > > /**< Total number of good bytes received from loopback,VF Only */ > > uint64_t olbbytes; > > /**< Total number of good bytes transmitted to loopback,VF > > Only */ > > > > + uint64_t imacerr; /**< Total of RX packets with MAC Errors. */ > > + uint64_t iphyerr; /**< Total of RX packets with PHY Errors. */ > > + uint64_t idrop; /**< Total number of dropped received packets. */ > > + uint64_t odrop; /**< Total number of dropped transmitted > > + packets. */ > > }; > > You are extending the generic stats. This is not the idea behind xstats. > The xstats are specific to the driver. I'd followed the example of: http://patchwork.dpdk.org/dev/patchwork/patch/85/ to added generic extended stats (at least what I thought were generic). I think that dropped packets should fall under struct rte_eth_stats, and should perhaps be left there, as most NICs/drivers should be able to provide that number. Would this be an agreeable solution? I have no other way to expose the total MAC errors and the total PHY errors without Adding counters into struct ixgbe_hw_stats, but I wasn't sure if this was allowable, is it? The only other option is to round up all the errors into ierrors, without having the granularity of what errors fall under. Is the latter option to sum up the values under one umbrella preferred? > Furthermore we should migrate some "not really generic stats" to xstats in > order to keep only the really basic and common stats in rte_eth_stats. > By the way, in order to avoid duplicated code when getting generic stats > through xstats API, we need to change the implementation of > rte_eth_xstats_get() to add generic stats automatically, even if the driver > provide some xstats.
[dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
Hello Maryam, On 06/22/2015 04:12 PM, Tahhan, Maryam wrote: > >> 2015-06-05 18:35, Maryam Tahhan: >>> Extend rte_eth_xstats_get to retrieve additional stats from the device >>> driver as well the top level extended stats. Add additional drop >>> counters to the extended stats. >>> >>> Signed-off-by: Maryam Tahhan >> [..] >> Patch 1/4 doesn't compile without patch 2/4. > > The rebased patches should fix this issue. >> >>> --- a/lib/librte_ether/rte_ethdev.c >>> +++ b/lib/librte_ether/rte_ethdev.c >>> @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off >> rte_stats_strings[] = { >>> {"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)}, >>> {"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)}, >>> {"rx_errors", offsetof(struct rte_eth_stats, ierrors)}, >>> + {"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)}, >>> + {"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)}, >>> {"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)}, >>> {"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)}, >>> {"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)}, @@ -136,6 >>> +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] >> = { >>> {"rx_flow_control_xon", offsetof(struct rte_eth_stats, >> rx_pause_xon)}, >>> {"tx_flow_control_xoff", offsetof(struct rte_eth_stats, >> tx_pause_xoff)}, >>> {"rx_flow_control_xoff", offsetof(struct rte_eth_stats, >>> rx_pause_xoff)}, >>> + {"tx_drops", offsetof(struct rte_eth_stats, odrop)}, >>> + {"rx_drops", offsetof(struct rte_eth_stats, idrop)}, >>> }; >> [...] >>> --- a/lib/librte_ether/rte_ethdev.h >>> +++ b/lib/librte_ether/rte_ethdev.h >>> @@ -224,6 +224,10 @@ struct rte_eth_stats { >>> >>> /**< Total number of good bytes received from loopback,VF Only */ >>> uint64_t olbbytes; >>> /**< Total number of good bytes transmitted to loopback,VF >>> Only */ >>> >>> + uint64_t imacerr; /**< Total of RX packets with MAC Errors. */ >>> + uint64_t iphyerr; /**< Total of RX packets with PHY Errors. */ >>> + uint64_t idrop; /**< Total number of dropped received packets. */ >>> + uint64_t odrop; /**< Total number of dropped transmitted >>> + packets. */ >>> }; >> >> You are extending the generic stats. This is not the idea behind xstats. >> The xstats are specific to the driver. > > I'd followed the example of: > http://patchwork.dpdk.org/dev/patchwork/patch/85/ > to added generic extended stats (at least what I thought were generic). I > think that > dropped packets should fall under struct rte_eth_stats, and should perhaps be > left > there, as most NICs/drivers should be able to provide that number. Would this > be an > agreeable solution? > > I have no other way to expose the total MAC errors and the total PHY errors > without > Adding counters into struct ixgbe_hw_stats, but I wasn't sure if this was > allowable, is it? > > The only other option is to round up all the errors into ierrors, without > having the > granularity of what errors fall under. Is the latter option to sum up the > values under one > umbrella preferred? As Thomas explained, I think we should avoid adding fields in struct rte_eth_stats. This structure already contains statistics that are specific to some hardware drivers. We should try to go in the opposite direction and remove all the fields that do not apply to all nics (physical or virtual). For me, it would be only something like (rx, tx, rx_bytes, tx_bytes, rx_errors, tx_errors). The xstats framework allows you to add any driver or hardware specific stats and I think it's the proper place to add them for ixgbe. By the way, something could be modified in xstats framework. Today, the behavior is: - if ethdev->dev_ops->xstats_get != NULL, call it - else, dump the generic stats in xstats format A better behavior could be: - Always dump the generic stats in xstats format - and if thdev->dev_ops->xstats_get != NULL, add driver-specific stats This would avoid to duplicate code that dumps generic stats in all drivers. So, to summarize what I think should be done for stats/xstats: 1. modify xstats behavior as described above 2. add the xstats dev_ops in ixgbe, it will dump the new stats Bonus: 3. identify all the specific stats that could be removed from rte_eth_stats and start to mark them as deprecated. Regards, Olivier > >> Furthermore we should migrate some "not really generic stats" to xstats in >> order to keep only the really basic and common stats in rte_eth_stats. >> By the way, in order to avoid duplicated code when getting generic stats >> through xstats API, we need to change the implementation of >> rte_eth_xstats_get() to add generic stats automatically, even if the driver >> provide some xstats.
[dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
> >>> + packets. */ > >>> }; > >> > >> You are extending the generic stats. This is not the idea behind xstats. > >> The xstats are specific to the driver. > > > > I'd followed the example of: > > http://patchwork.dpdk.org/dev/patchwork/patch/85/ > > to added generic extended stats (at least what I thought were > > generic). I think that dropped packets should fall under struct > > rte_eth_stats, and should perhaps be left there, as most NICs/drivers > > should be able to provide that number. Would this be an agreeable > solution? > > > > I have no other way to expose the total MAC errors and the total PHY > > errors without Adding counters into struct ixgbe_hw_stats, but I wasn't sure > if this was allowable, is it? > > > > The only other option is to round up all the errors into ierrors, > > without having the granularity of what errors fall under. Is the > > latter option to sum up the values under one umbrella preferred? > > As Thomas explained, I think we should avoid adding fields in struct > rte_eth_stats. This structure already contains statistics that are specific to > some hardware drivers. We should try to go in the opposite direction and > remove all the fields that do not apply to all nics (physical or virtual). > For me, it > would be only something like (rx, tx, rx_bytes, tx_bytes, rx_errors, > tx_errors). > > The xstats framework allows you to add any driver or hardware specific stats > and I think it's the proper place to add them for ixgbe. > Alright that sounds good. > By the way, something could be modified in xstats framework. Today, the > behavior is: > - if ethdev->dev_ops->xstats_get != NULL, call it > - else, dump the generic stats in xstats format As it happens I did do this in: http://dpdk.org/dev/patchwork/patch/5324/ but I will reverse the order as what happens now is xstats from the driver are displayed first and will look at always dumping generic stats in xstats format as you mention below. > > A better behavior could be: > - Always dump the generic stats in xstats format > - and if thdev->dev_ops->xstats_get != NULL, add driver-specific stats > > This would avoid to duplicate code that dumps generic stats in all drivers. > > So, to summarize what I think should be done for stats/xstats: > 1. modify xstats behavior as described above 2. add the xstats dev_ops in > ixgbe, it will dump the new stats > > Bonus: > 3. identify all the specific stats that could be removed from >rte_eth_stats and start to mark them as deprecated. > Will do. > > Regards, > Olivier Thanks Maryam