From: "David Harton (dharton)" <dharton@cisco.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] ixgbe: add counter to track sw tx packets
Date: Tue, 29 Aug 2017 19:50:43 +0000 [thread overview]
Message-ID: <36c561582a4f4d7f9ed27d3cb39fd191@XCH-RCD-016.cisco.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB9772584F23DC98@IRSMSX103.ger.corp.intel.com>
> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Tuesday, August 29, 2017 3:29 PM
> To: David Harton (dharton) <dharton@cisco.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH] ixgbe: add counter to track sw tx packets
>
>
>
> > -----Original Message-----
> > From: David Harton [mailto:dharton@cisco.com]
> > Sent: Tuesday, August 29, 2017 4:51 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: dev@dpdk.org; David Harton <dharton@cisco.com>
> > Subject: [PATCH] ixgbe: add counter to track sw tx packets
> >
> > Add counter to track packets transmitted at the software layer to help
> > isolate output errors not reported otherwise.
>
>
> So what is the reason why same stats couldn't be collected at the
> application layer?
> I.E:
> nb_tx = rte_eth_tx_bulk(port, queue, ...); sw_stats[...]->tx_pkts +=
> nb_tx; ?
> That's' generic way that would work for all PMDs (not ixgbe only) and
> wouldn't affect actual PMD TX function in any way.
> Konstantin
The motivation is to have rte_eth_stats_get() report the advertised output errors along with providing some insight to where the drops are happening with xstats (i.e. happening after passing the DPDK boundary).
Are you asking that DPDK track the tx stats in librte_ether so we can report oerrors? The problem with that (similar to tracking at the application layer) is some drivers do report oerrors so how does that layer know when it should or shouldn't provide that stat?
If the concern is cache thrashing as Stephen implies then the stats could be made per Q.
Thanks,
Dave
>
> >
> > Signed-off-by: David Harton <dharton@cisco.com>
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 98
> > ++++++++++++++++++++++++++++++++++++----
> > drivers/net/ixgbe/ixgbe_ethdev.h | 9 ++++
> > drivers/net/ixgbe/ixgbe_rxtx.c | 49 ++++++++++++++------
> > 3 files changed, 132 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index ed21af5..a38a595 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -728,6 +728,14 @@ struct rte_ixgbe_xstats_name_off { #define
> > IXGBE_NB_HW_STATS (sizeof(rte_ixgbe_stats_strings) / \
> > sizeof(rte_ixgbe_stats_strings[0]))
> >
> > +/* SW statistics */
> > +static const struct rte_ixgbe_xstats_name_off rte_ixgbe_sw_strings[] =
> {
> > + {"tx_pkts", offsetof(struct ixgbe_sw_stats, tx_pkts)}, };
> > +
> > +#define IXGBE_NB_SW_STATS (sizeof(rte_ixgbe_sw_strings) / \
> > + sizeof(rte_ixgbe_sw_strings[0]))
> > +
> > /* MACsec statistics */
> > static const struct rte_ixgbe_xstats_name_off
> rte_ixgbe_macsec_strings[] = {
> > {"out_pkts_untagged", offsetof(struct ixgbe_macsec_stats, @@ -3085,6
> > +3093,8 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device
> *pci_dev)
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > struct ixgbe_hw_stats *hw_stats =
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats =
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> > struct ixgbe_macsec_stats *macsec_stats =
> > IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(
> > dev->data->dev_private);
> > @@ -3130,7 +3140,10 @@ static int eth_ixgbevf_pci_remove(struct
> rte_pci_device *pci_dev)
> > hw_stats->fclast;
> >
> > /* Tx Errors */
> > - stats->oerrors = 0;
> > + if (sw_stats->tx_pkts > stats->opackets)
> > + stats->oerrors = sw_stats->tx_pkts - stats->opackets;
> > + else
> > + stats->oerrors = 0;
> > }
> >
> > static void
> > @@ -3138,18 +3151,21 @@ static int eth_ixgbevf_pci_remove(struct
> > rte_pci_device *pci_dev) {
> > struct ixgbe_hw_stats *stats =
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats =
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> >
> > /* HW registers are cleared on read */
> > ixgbe_dev_stats_get(dev, NULL);
> >
> > /* Reset software totals */
> > memset(stats, 0, sizeof(*stats));
> > + memset(sw_stats, 0, sizeof(*sw_stats));
> > }
> >
> > /* This function calculates the number of xstats based on the current
> > config */ static unsigned
> > ixgbe_xstats_calc_num(void) {
> > - return IXGBE_NB_HW_STATS + IXGBE_NB_MACSEC_STATS +
> > + return IXGBE_NB_HW_STATS + IXGBE_NB_MACSEC_STATS + IXGBE_NB_SW_STATS
> > ++
> > (IXGBE_NB_RXQ_PRIO_STATS * IXGBE_NB_RXQ_PRIO_VALUES) +
> > (IXGBE_NB_TXQ_PRIO_STATS * IXGBE_NB_TXQ_PRIO_VALUES); } @@ -
> 3176,6
> > +3192,15 @@ static int ixgbe_dev_xstats_get_names(__rte_unused struct
> rte_eth_dev *dev,
> > count++;
> > }
> >
> > + /* SW Stats */
> > + for (i = 0; i < IXGBE_NB_SW_STATS; i++) {
> > + snprintf(xstats_names[count].name,
> > + sizeof(xstats_names[count].name),
> > + "%s",
> > + rte_ixgbe_sw_strings[i].name);
> > + count++;
> > + }
> > +
> > /* MACsec Stats */
> > for (i = 0; i < IXGBE_NB_MACSEC_STATS; i++) {
> > snprintf(xstats_names[count].name,
> > @@ -3236,6 +3261,15 @@ static int ixgbe_dev_xstats_get_names_by_id(
> > count++;
> > }
> >
> > + /* SW Stats */
> > + for (i = 0; i < IXGBE_NB_SW_STATS; i++) {
> > + snprintf(xstats_names[count].name,
> > + sizeof(xstats_names[count].name),
> > + "%s",
> > + rte_ixgbe_sw_strings[i].name);
> > + count++;
> > + }
> > +
> > /* MACsec Stats */
> > for (i = 0; i < IXGBE_NB_MACSEC_STATS; i++) {
> > snprintf(xstats_names[count].name,
> > @@ -3291,17 +3325,23 @@ static int ixgbe_dev_xstats_get_names_by_id(
> > static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev
> *dev,
> > struct rte_eth_xstat_name *xstats_names, unsigned limit) {
> > - unsigned i;
> > + unsigned int i, j;
> >
> > - if (limit < IXGBEVF_NB_XSTATS && xstats_names != NULL)
> > + if (limit < (IXGBEVF_NB_XSTATS + IXGBE_NB_SW_STATS) &&
> > + xstats_names != NULL)
> > return -ENOMEM;
> >
> > - if (xstats_names != NULL)
> > + if (xstats_names != NULL) {
> > for (i = 0; i < IXGBEVF_NB_XSTATS; i++)
> > snprintf(xstats_names[i].name,
> > sizeof(xstats_names[i].name),
> > "%s", rte_ixgbevf_stats_strings[i].name);
> > - return IXGBEVF_NB_XSTATS;
> > + for (j = 0; j < IXGBE_NB_SW_STATS; j++, i++)
> > + snprintf(xstats_names[i].name,
> > + sizeof(xstats_names[i].name),
> > + "%s", rte_ixgbe_sw_strings[j].name);
> > + }
> > + return (IXGBEVF_NB_XSTATS + IXGBE_NB_SW_STATS);
> > }
> >
> > static int
> > @@ -3312,6 +3352,8 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > struct ixgbe_hw_stats *hw_stats =
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats =
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> > struct ixgbe_macsec_stats *macsec_stats =
> > IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(
> > dev->data->dev_private);
> > @@ -3346,6 +3388,14 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > count++;
> > }
> >
> > + /* SW Stats */
> > + for (i = 0; i < IXGBE_NB_SW_STATS; i++) {
> > + xstats[count].value = *(uint64_t *)(((char *)sw_stats) +
> > + rte_ixgbe_sw_strings[i].offset);
> > + xstats[count].id = count;
> > + count++;
> > + }
> > +
> > /* MACsec Stats */
> > for (i = 0; i < IXGBE_NB_MACSEC_STATS; i++) {
> > xstats[count].value = *(uint64_t *)(((char *)macsec_stats) +
> @@
> > -3388,6 +3438,9 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> > struct ixgbe_hw_stats *hw_stats =
> > IXGBE_DEV_PRIVATE_TO_STATS(
> > dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats =
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(
> > + dev->data->dev_private);
> > struct ixgbe_macsec_stats *macsec_stats =
> > IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(
> > dev->data->dev_private);
> > @@ -3422,6 +3475,13 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > count++;
> > }
> >
> > + /* SW Stats */
> > + for (i = 0; i < IXGBE_NB_SW_STATS; i++) {
> > + values[count] = *(uint64_t *)(((char *)sw_stats) +
> > + rte_ixgbe_sw_strings[i].offset);
> > + count++;
> > + }
> > +
> > /* MACsec Stats */
> > for (i = 0; i < IXGBE_NB_MACSEC_STATS; i++) {
> > values[count] = *(uint64_t *)(((char *)macsec_stats) +
> @@ -3522,10
> > +3582,12 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
> > struct rte_eth_dev *dev, {
> > struct ixgbevf_hw_stats *hw_stats = (struct ixgbevf_hw_stats *)
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > - unsigned i;
> > + struct ixgbe_sw_stats *sw_stats = (struct ixgbe_sw_stats *)
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> > + unsigned int i, j;
> >
> > - if (n < IXGBEVF_NB_XSTATS)
> > - return IXGBEVF_NB_XSTATS;
> > + if (n < (IXGBEVF_NB_XSTATS + IXGBE_NB_SW_STATS))
> > + return (IXGBEVF_NB_XSTATS + IXGBE_NB_SW_STATS);
> >
> > ixgbevf_update_stats(dev);
> >
> > @@ -3538,8 +3600,13 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > xstats[i].value = *(uint64_t *)(((char *)hw_stats) +
> > rte_ixgbevf_stats_strings[i].offset);
> > }
> > + for (j = 0; j < IXGBE_NB_SW_STATS; j++, i++) {
> > + xstats[i].id = i;
> > + xstats[i].value = *(uint64_t *)(((char *)sw_stats) +
> > + rte_ixgbe_sw_strings[j].offset);
> > + }
> >
> > - return IXGBEVF_NB_XSTATS;
> > + return (IXGBEVF_NB_XSTATS + IXGBE_NB_SW_STATS);
> > }
> >
> > static void
> > @@ -3547,6 +3614,8 @@ static int
> > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev, {
> > struct ixgbevf_hw_stats *hw_stats = (struct ixgbevf_hw_stats *)
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats = (struct ixgbe_sw_stats *)
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> >
> > ixgbevf_update_stats(dev);
> >
> > @@ -3557,6 +3626,11 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > stats->ibytes = hw_stats->vfgorc;
> > stats->opackets = hw_stats->vfgptc;
> > stats->obytes = hw_stats->vfgotc;
> > +
> > + if (sw_stats->tx_pkts > stats->opackets)
> > + stats->oerrors = sw_stats->tx_pkts - stats->opackets;
> > + else
> > + stats->oerrors = 0;
> > }
> >
> > static void
> > @@ -3564,6 +3638,8 @@ static int
> > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev, {
> > struct ixgbevf_hw_stats *hw_stats = (struct ixgbevf_hw_stats *)
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats = (struct ixgbe_sw_stats *)
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> >
> > /* Sync HW register to the last stats */
> > ixgbevf_dev_stats_get(dev, NULL);
> > @@ -3573,6 +3649,8 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > hw_stats->vfgorc = 0;
> > hw_stats->vfgptc = 0;
> > hw_stats->vfgotc = 0;
> > +
> > + memset(sw_stats, 0, sizeof(*sw_stats));
> > }
> >
> > static int
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> > b/drivers/net/ixgbe/ixgbe_ethdev.h
> > index 3f1aeb5..ba9350c 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> > @@ -434,6 +434,11 @@ struct ixgbe_macsec_stats {
> > uint64_t in_pkts_notusingsa;
> > };
> >
> > +/* Struct to track emulated stats */
> > +struct ixgbe_sw_stats {
> > + uint64_t tx_pkts;
> > +};
> > +
> > /* The configuration of bandwidth */
> > struct ixgbe_bw_conf {
> > uint8_t tc_num; /* Number of TCs. */ @@ -508,6 +513,7 @@ struct
> > ixgbe_adapter {
> > struct ixgbe_hw hw;
> > struct ixgbe_hw_stats stats;
> > struct ixgbe_macsec_stats macsec_stats;
> > + struct ixgbe_sw_stats sw_stats;
> > struct ixgbe_hw_fdir_info fdir;
> > struct ixgbe_interrupt intr;
> > struct ixgbe_stat_mapping_registers stat_mappings; @@ -541,6 +547,9
> > @@ struct ixgbe_adapter { #define
> > IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
> > (&((struct ixgbe_adapter *)adapter)->macsec_stats)
> >
> > +#define IXGBE_DEV_PRIVATE_TO_SW_STATS(adapter) \
> > + (&((struct ixgbe_adapter *)adapter)->sw_stats)
> > +
> > #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
> > (&((struct ixgbe_adapter *)adapter)->intr)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > b/drivers/net/ixgbe/ixgbe_rxtx.c index 64bff25..34e3968 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -347,24 +347,35 @@ uint16_t ixgbe_xmit_fixed_burst_vec(void
> *tx_queue, struct rte_mbuf **tx_pkts,
> > uint16_t nb_pkts)
> > {
> > uint16_t nb_tx;
> > + struct ixgbe_tx_queue *txq = (struct ixgbe_tx_queue *)tx_queue;
> > + struct ixgbe_sw_stats *sw_stats = (struct ixgbe_sw_stats *)
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(
> > + rte_eth_devices[txq->port_id].data->dev_private);
> >
> > /* Try to transmit at least chunks of TX_MAX_BURST pkts */
> > - if (likely(nb_pkts <= RTE_PMD_IXGBE_TX_MAX_BURST))
> > - return tx_xmit_pkts(tx_queue, tx_pkts, nb_pkts);
> > -
> > - /* transmit more than the max burst, in chunks of TX_MAX_BURST */
> > - nb_tx = 0;
> > - while (nb_pkts) {
> > - uint16_t ret, n;
> > -
> > - n = (uint16_t)RTE_MIN(nb_pkts, RTE_PMD_IXGBE_TX_MAX_BURST);
> > - ret = tx_xmit_pkts(tx_queue, &(tx_pkts[nb_tx]), n);
> > - nb_tx = (uint16_t)(nb_tx + ret);
> > - nb_pkts = (uint16_t)(nb_pkts - ret);
> > - if (ret < n)
> > - break;
> > + if (likely(nb_pkts <= RTE_PMD_IXGBE_TX_MAX_BURST)) {
> > + nb_tx = tx_xmit_pkts(tx_queue, tx_pkts, nb_pkts);
> > + } else {
> > + /*
> > + * transmit more than the max burst,
> > + * in chunks of TX_MAX_BURST
> > + */
> > + nb_tx = 0;
> > + while (nb_pkts) {
> > + uint16_t ret, n;
> > +
> > + n = (uint16_t)RTE_MIN(
> > + nb_pkts, RTE_PMD_IXGBE_TX_MAX_BURST);
> > + ret = tx_xmit_pkts(tx_queue, &tx_pkts[nb_tx], n);
> > + nb_tx = (uint16_t)(nb_tx + ret);
> > + nb_pkts = (uint16_t)(nb_pkts - ret);
> > + if (ret < n)
> > + break;
> > + }
> > }
> >
> > + sw_stats->tx_pkts += nb_tx;
> > +
> > return nb_tx;
> > }
> >
> > @@ -375,6 +386,9 @@ uint16_t ixgbe_xmit_fixed_burst_vec(void
> > *tx_queue, struct rte_mbuf **tx_pkts, {
> > uint16_t nb_tx = 0;
> > struct ixgbe_tx_queue *txq = (struct ixgbe_tx_queue *)tx_queue;
> > + struct ixgbe_sw_stats *sw_stats = (struct ixgbe_sw_stats *)
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(
> > + rte_eth_devices[txq->port_id].data->dev_private);
> >
> > while (nb_pkts) {
> > uint16_t ret, num;
> > @@ -388,6 +402,8 @@ uint16_t ixgbe_xmit_fixed_burst_vec(void *tx_queue,
> struct rte_mbuf **tx_pkts,
> > break;
> > }
> >
> > + sw_stats->tx_pkts += nb_tx;
> > +
> > return nb_tx;
> > }
> > #endif
> > @@ -636,6 +652,7 @@ uint16_t ixgbe_xmit_fixed_burst_vec(void
> > *tx_queue, struct rte_mbuf **tx_pkts, ixgbe_xmit_pkts(void *tx_queue,
> struct rte_mbuf **tx_pkts,
> > uint16_t nb_pkts)
> > {
> > + struct ixgbe_sw_stats *sw_stats;
> > struct ixgbe_tx_queue *txq;
> > struct ixgbe_tx_entry *sw_ring;
> > struct ixgbe_tx_entry *txe, *txn;
> > @@ -942,6 +959,10 @@ uint16_t ixgbe_xmit_fixed_burst_vec(void *tx_queue,
> struct rte_mbuf **tx_pkts,
> > IXGBE_PCI_REG_WRITE_RELAXED(txq->tdt_reg_addr, tx_id);
> > txq->tx_tail = tx_id;
> >
> > + sw_stats = (struct ixgbe_sw_stats *)IXGBE_DEV_PRIVATE_TO_SW_STATS(
> > + rte_eth_devices[txq->port_id].data->dev_private);
> > + sw_stats->tx_pkts += nb_tx;
> > +
> > return nb_tx;
> > }
> >
> > --
> > 1.8.3.1
next prev parent reply other threads:[~2017-08-29 19:50 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-29 15:50 David Harton
2017-08-29 18:42 ` Stephen Hemminger
2017-08-29 19:20 ` David Harton (dharton)
2017-08-29 18:44 ` Stephen Hemminger
2017-08-29 19:29 ` Ananyev, Konstantin
2017-08-29 19:50 ` David Harton (dharton) [this message]
2017-08-30 14:27 ` Ananyev, Konstantin
2017-08-30 15:32 ` David Harton (dharton)
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=36c561582a4f4d7f9ed27d3cb39fd191@XCH-RCD-016.cisco.com \
--to=dharton@cisco.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@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).