Re: [PATCH] ixgbe: add counter to track sw tx packets

From: David Harton (dharton) <hidden>
Date: 2017-08-29 19:50:45

-----Original Message-----
From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
Sent: Tuesday, August 29, 2017 3:29 PM
To: David Harton (dharton) <redacted>
Cc: dev@dpdk.org
Subject: RE: [PATCH] ixgbe: add counter to track sw tx packets

quoted

-----Original Message-----
From: David Harton [mailto:dharton@cisco.com]
Sent: Tuesday, August 29, 2017 4:51 PM
To: Ananyev, Konstantin <redacted>
Cc: dev@dpdk.org; David Harton <redacted>
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

quoted hunk ↗ jump to hunk

quoted

Signed-off-by: David Harton <redacted>
---
 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[] =

quoted

+	{"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[] = {

quoted

 	{"out_pkts_untagged", offsetof(struct ixgbe_macsec_stats, @@ -3085,6
+3093,8 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device

*pci_dev)

quoted

 			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)

quoted

 			  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

quoted

+3192,15 @@ static int ixgbe_dev_xstats_get_names(__rte_unused struct

rte_eth_dev *dev,

quoted

 			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,

quoted

 	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,

quoted

 			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,

quoted

 		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) +

@@

quoted

-3388,6 +3438,9 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused

struct rte_eth_dev *dev,

quoted

 		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,

quoted

 			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

quoted

+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,

quoted

 		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,

quoted

 	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,

quoted

 	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,

quoted

 		       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,

quoted

 			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,

quoted

 		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,

quoted

 	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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help