From: "Tahhan, Maryam" <maryam.tahhan@intel.com>
To: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 2/7] ixgbe: add functions to get and reset xstats
Date: Sun, 5 Jul 2015 09:27:12 +0000 [thread overview]
Message-ID: <1A27633A6DA49C4A92FCD5D4312DBF536A45ADC2@IRSMSX109.ger.corp.intel.com> (raw)
In-Reply-To: <55968B1C.6070405@6wind.com>
Best Regards,
Maryam
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, July 3, 2015 2:16 PM
> To: Tahhan, Maryam; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/7] ixgbe: add functions to get and reset
> xstats
>
> Hi Maryam,
>
> On 06/26/2015 02:59 PM, Maryam Tahhan wrote:
> > Implement ixgbe_dev_xstats_reset and ixgbe_dev_xstats_get.
> >
> > Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 85
> > ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 85 insertions(+)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 927021f..0f62bcb 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -131,7 +131,10 @@ static int ixgbe_dev_link_update(struct
> rte_eth_dev *dev,
> > int wait_to_complete);
> > static void ixgbe_dev_stats_get(struct rte_eth_dev *dev,
> > struct rte_eth_stats *stats);
> > +static int ixgbe_dev_xstats_get(struct rte_eth_dev *dev,
> > + struct rte_eth_xstats *xstats, unsigned n);
> > static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev);
> > +static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev);
> > static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev
> *eth_dev,
> > uint16_t queue_id,
> > uint8_t stat_idx,
> > @@ -334,7 +337,9 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops
> = {
> > .allmulticast_disable = ixgbe_dev_allmulticast_disable,
> > .link_update = ixgbe_dev_link_update,
> > .stats_get = ixgbe_dev_stats_get,
> > + .xstats_get = ixgbe_dev_xstats_get,
> > .stats_reset = ixgbe_dev_stats_reset,
> > + .xstats_reset = ixgbe_dev_xstats_reset,
> > .queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set,
> > .dev_infos_get = ixgbe_dev_info_get,
> > .mtu_set = ixgbe_dev_mtu_set,
> > @@ -414,6 +419,33 @@ static const struct eth_dev_ops
> ixgbevf_eth_dev_ops = {
> > .set_mc_addr_list = ixgbe_dev_set_mc_addr_list,
> > };
> >
> > +/* store statistics names and its offset in stats structure */
> > +struct rte_ixgbe_xstats_name_off {
> > + char name[RTE_ETH_XSTATS_NAME_SIZE];
> > + unsigned offset;
> > +};
> > +
> > +static const struct rte_ixgbe_xstats_name_off rte_ixgbe_stats_strings[] = {
> > + {"rx_illegal_byte_err", offsetof(struct ixgbe_hw_stats, errbc)},
> > + {"rx_len_err", offsetof(struct ixgbe_hw_stats, rlec)},
> > + {"rx_undersize_count", offsetof(struct ixgbe_hw_stats, ruc)},
> > + {"rx_oversize_count", offsetof(struct ixgbe_hw_stats, roc)},
> > + {"rx_fragment_count", offsetof(struct ixgbe_hw_stats, rfc)},
> > + {"rx_jabber_count", offsetof(struct ixgbe_hw_stats, rjc)},
> > + {"l3_l4_xsum_error", offsetof(struct ixgbe_hw_stats, xec)},
> > + {"mac_local_fault", offsetof(struct ixgbe_hw_stats, mlfc)},
> > + {"mac_remote_fault", offsetof(struct ixgbe_hw_stats, mrfc)},
> > + {"mac_short_pkt_discard", offsetof(struct ixgbe_hw_stats, mspdc)},
> > + {"fccrc_error", offsetof(struct ixgbe_hw_stats, fccrc)},
> > + {"fcoe_drop", offsetof(struct ixgbe_hw_stats, fcoerpdc)},
> > + {"fc_last_error", offsetof(struct ixgbe_hw_stats, fclast)},
> > + {"rx_broadcast_packets", offsetof(struct ixgbe_hw_stats, bprc)},
> > + {"mgmt_pkts_dropped", offsetof(struct ixgbe_hw_stats, mngpdc)}, };
> > +
> > +#define RTE_NB_XSTATS (sizeof(rte_ixgbe_stats_strings) / \
> > + sizeof(rte_ixgbe_stats_strings[0]))
> > +
>
> Maybe RTE_NB_XSTATS should be renamed in IXGBE_NB_XSTATS?
>
>
> > /**
> > * Atomically reads the link status information from global
> > * structure rte_eth_dev.
> > @@ -1982,6 +2014,59 @@ ixgbe_dev_stats_reset(struct rte_eth_dev
> *dev)
> > memset(stats, 0, sizeof(*stats));
> > }
> >
> > +static int
> > +ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats
> *xstats,
> > + unsigned n)
> > +{
> > + struct ixgbe_hw *hw =
> > + IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> > + struct ixgbe_hw_stats *hw_stats =
> > + IXGBE_DEV_PRIVATE_TO_STATS(dev->data-
> >dev_private);
> > + uint64_t total_missed_rx, total_qbrc, total_qprc, total_qprdc;
> > + uint64_t rxnfgpc, txdgpc;
> > + unsigned i, count = RTE_NB_XSTATS;
> > +
> > + if (n == 0)
> > + return count;
>
> I think it does not exactly matches the API described in rte_ethdev.h:
>
> * @return
> * - positive value lower or equal to n: success. The return value
> * is the number of entries filled in the stats table.
> * - positive value higher than n: error, the given statistics table
> * is too small. The return value corresponds to the size that should
> * be given to succeed. The entries in the table are not valid and
> * shall not be used by the caller.
> * - negative value on error (invalid port id)
>
> I think it should be:
>
> if (n < count)
> return count
>
I was using 0 on the first call just to return the size of the extended stats structure that needs to be allocated by the application. It's passed as 0 from ethdev_get_stats to retrieve the size. But I will update it.
>
> > +
> > + total_missed_rx = 0;
> > + total_qbrc = 0;
> > + total_qprc = 0;
> > + total_qprdc = 0;
> > + rxnfgpc = 0;
> > + txdgpc = 0;
> > + count = 0;
> > +
> > + ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx,
> &total_qbrc,
> > + &total_qprc,
> &rxnfgpc, &txdgpc, &total_qprdc);
> > +
> > + if (!xstats)
> > + return 0;
>
This is the mechanism used to reset stats for normal stats and xstats. The reset functions call the stats function with the nullified stats structure. So I think this is ok to leave in, I will add a comment about it being part of the reset functionality
> this cannot happen except if n == 0.
> This condition is already tested above, and "count" should be returned.
>
> > +
> > + /* Error stats */
> > + for (i = 0; i < RTE_NB_XSTATS; i++) {
> > + snprintf(xstats[count].name, sizeof(xstats[count].name),
> > + "%s", rte_ixgbe_stats_strings[i].name);
> > + xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
> > +
> rte_ixgbe_stats_strings[i].offset);
> > + }
> > +
> > + return count;
> > +}
>
> Shouldn't it be xstats[i] instead of xstats[count] ?
No, i is just used to cycle through the stats string structure but we use the count for xstats because that is the index that is passed in from rte_eth_xstats_get(). Remember we are filling out the xstats structure with generic stats at the ethdev level first and then passing the xstats structure to the driver for the rest of it to be filled out.
>
> Does it work when using "show port in test-pmd"?
Yes, I tested it before patch submission and it works.
>
> > +
> > +static void
> > +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev) {
> > + struct ixgbe_hw_stats *stats =
> > + IXGBE_DEV_PRIVATE_TO_STATS(dev->data-
> >dev_private);
> > +
> > + /* HW registers are cleared on read */
> > + ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS);
> > +
> > + /* Reset software totals */
> > + memset(stats, 0, sizeof(*stats));
> > +}
> > +
> > static void
> > ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> > *stats) {
> >
next prev parent reply other threads:[~2015-07-05 9:27 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-26 12:59 [dpdk-dev] [PATCH v3 0/7] Expose IXGBE extended stats to DPDK apps Maryam Tahhan
2015-06-26 12:59 ` [dpdk-dev] [PATCH v3 1/7] ixgbe: move stats register reads to a new function Maryam Tahhan
2015-06-26 12:59 ` [dpdk-dev] [PATCH v3 2/7] ixgbe: add functions to get and reset xstats Maryam Tahhan
2015-07-03 13:16 ` Olivier MATZ
2015-07-03 13:19 ` Olivier MATZ
2015-07-05 9:34 ` Tahhan, Maryam
2015-07-05 10:02 ` Tahhan, Maryam
2015-07-05 9:27 ` Tahhan, Maryam [this message]
2015-06-26 12:59 ` [dpdk-dev] [PATCH v3 3/7] ethdev: expose extended error stats Maryam Tahhan
2015-07-03 13:27 ` Olivier MATZ
2015-06-26 12:59 ` [dpdk-dev] [PATCH v3 4/7] ethdev: remove HW specific stats in stats structs Maryam Tahhan
2015-06-26 14:03 ` Kyle Larose
2015-06-26 14:30 ` Tahhan, Maryam
2015-06-26 14:37 ` Kyle Larose
2015-06-26 14:47 ` Tahhan, Maryam
2015-06-26 12:59 ` [dpdk-dev] [PATCH v3 5/7] ixgbe: add NIC specific stats removed from ethdev Maryam Tahhan
2015-06-26 12:59 ` [dpdk-dev] [PATCH v3 6/7] app: remove dump_cfg Maryam Tahhan
2015-06-26 12:59 ` [dpdk-dev] [PATCH v3 7/7] app: add a new app proc_info Maryam Tahhan
2015-07-01 14:27 ` [dpdk-dev] [PATCH v3 0/7] Expose IXGBE extended stats to DPDK apps Tahhan, Maryam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1A27633A6DA49C4A92FCD5D4312DBF536A45ADC2@IRSMSX109.ger.corp.intel.com \
--to=maryam.tahhan@intel.com \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).