From: Olivier MATZ <olivier.matz@6wind.com>
To: Maryam Tahhan <maryam.tahhan@intel.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stats
Date: Wed, 15 Jul 2015 11:18:58 +0200 [thread overview]
Message-ID: <55A62582.9090208@6wind.com> (raw)
In-Reply-To: <1436118000-132275-4-git-send-email-maryam.tahhan@intel.com>
Hi Maryam,
The API of the driver-specific function should be the same than
the generic one, described in rte_ethdev.h.
What I mean is:
- the xstats pointer is the place where the xstats should be written
by the driver
- the n argument is the number of entries in the xstats structure
The driver should not have to worry about the generic stats written
by the generic layer.
Please see some comments below to fix it.
On 07/05/2015 07:39 PM, Maryam Tahhan wrote:
> Extend rte_eth_xstats_get to retrieve additional stats from the device
> driver as well the ethdev generic stats.
>
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 7 ++++---
> lib/librte_ether/rte_ethdev.c | 25 ++++++++++++++++---------
> 2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 7feb03c..5971d41 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2035,6 +2035,7 @@ ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
> total_qprdc = 0;
> rxnfgpc = 0;
> txdgpc = 0;
> + count = n - IXGBE_NB_XSTATS;
>
> ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, &total_qbrc,
> &total_qprc, &rxnfgpc, &txdgpc, &total_qprdc);
> @@ -2047,13 +2048,13 @@ ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
>
> /* Extended stats */
> for (i = 0; i < IXGBE_NB_XSTATS; i++) {
> - snprintf(xstats[i].name, sizeof(xstats[i].name),
> + snprintf(xstats[count].name, sizeof(xstats[i].name),
> "%s", rte_ixgbe_stats_strings[i].name);
> - xstats[i].value = *(uint64_t *)(((char *)hw_stats) +
> + xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
> rte_ixgbe_stats_strings[i].offset);
> }
>
> - return count;
> + return IXGBE_NB_XSTATS;
The modifications in ixgbe driver can be removed: the patch 2/8 already
provides everything that is required.
> }
>
> static void
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index da915db..e392f60 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1681,26 +1681,33 @@ 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, i, q;
> + signed xcount = 0;
> uint64_t val, *stats_ptr;
>
> VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>
> dev = &rte_eth_devices[port_id];
>
> - /* implemented by the driver */
> - if (dev->dev_ops->xstats_get != NULL)
> - return (*dev->dev_ops->xstats_get)(dev, xstats, n);
> -
> - /* else, return generic statistics */
> + /* 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;
> +
> + /* implemented by the driver */
> + if (dev->dev_ops->xstats_get != NULL) {
> + /* Retrieve the xstats from the driver at the end of the
> + * xstats struct.
> + */
> + xcount = (*dev->dev_ops->xstats_get)(dev, xstats, n);
it should be:
xcount = (*dev->dev_ops->xstats_get)(dev, &xstats[count],
(n > count) ? n - count : 0);
> + if (xcount < 0)
> + return xcount;
> + }
> +
> if (n < count)
> - return count;
> + return count + xcount;
it should be:
if (n < count + xcount)
return count + xcount;
>
> /* now fill the xstats structure */
> -
> count = 0;
> rte_eth_stats_get(port_id, ð_stats);
>
> @@ -1742,7 +1749,7 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
> }
> }
>
> - return count;
> + return count + xcount;
> }
>
> /* reset ethdev extended statistics */
>
Regards,
Olivier
next prev parent reply other threads:[~2015-07-15 9:19 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-05 17:39 [dpdk-dev] [PATCH v4 0/8] Expose IXGBE extended stats to DPDK apps Maryam Tahhan
2015-07-05 17:39 ` [dpdk-dev] [PATCH v4 1/8] ixgbe: move stats register reads to a new function Maryam Tahhan
2015-07-05 17:39 ` [dpdk-dev] [PATCH v4 2/8] ixgbe: add functions to get and reset xstats Maryam Tahhan
2015-07-05 17:39 ` [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stats Maryam Tahhan
2015-07-15 9:18 ` Olivier MATZ [this message]
2015-07-15 13:14 ` Tahhan, Maryam
2015-07-05 17:39 ` [dpdk-dev] [PATCH v4 4/8] ethdev: remove HW specific stats in stats structs Maryam Tahhan
2015-07-05 17:39 ` [dpdk-dev] [PATCH v4 5/8] ixgbe: add NIC specific stats removed from ethdev Maryam Tahhan
2015-07-05 17:39 ` [dpdk-dev] [PATCH v4 6/8] ixgbe: return more errors in ierrors Maryam Tahhan
2015-07-05 17:39 ` [dpdk-dev] [PATCH v4 7/8] app: remove dump_cfg Maryam Tahhan
2015-07-05 17:40 ` [dpdk-dev] [PATCH v4 8/8] app: add a new app proc_info Maryam Tahhan
2015-07-09 1:39 ` Thomas Monjalon
2015-07-13 13:30 ` 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=55A62582.9090208@6wind.com \
--to=olivier.matz@6wind.com \
--cc=dev@dpdk.org \
--cc=maryam.tahhan@intel.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).