Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 13 Feb 2019 14:36:24 +0000 (UTC)
From:      Marius Strobl <marius@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org
Subject:   svn commit: r344097 - in stable/12/sys/dev: e1000 ixgbe ixl
Message-ID:  <201902131436.x1DEaOM7040710@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: marius
Date: Wed Feb 13 14:36:24 2019
New Revision: 344097
URL: https://svnweb.freebsd.org/changeset/base/344097

Log:
  MFC: r343369
  
  intel iflib drivers: correct initialization of tx_cidx_processed
  
  From Jake:
  
  In r341156 ("Fix first-packet completion", 2018-11-28) a hack to work
  around a delta calculation determining how many descriptors were used
  was added to ixl_isc_tx_credits_update_dwb.
  
  The same fix was also applied to the em and igb drivers in r340310, and
  to ix in r341156.
  
  The hack checked the case where prev and cur were equal, and then added
  one. This works, because by the time we do the delta check, we already
  know there is at least one packet available, so the delta should be at
  least one.
  
  However, it's not a complete fix, and as indicated by the comment is
  really a hack to work around the real bug.
  
  The real problem is that the first time that we transmit a packet,
  tx_cidx_processed will be set to point to the start of the ring.
  Ultimately, the credits_update function expects it to point to the
  *last* descriptor that was processed. Since we haven't yet processed any
  descriptors, pointing it to 0 results in this incorrect calculation.
  
  Fix the initialization code to have it point to the end of the ring
  instead. One way to think about this, is that we are setting the value
  to be one prior to the first available descriptor.
  
  Doing so, corrects the delta calculation in all cases. The original fix
  only works if the first packet has exactly one descriptor. Otherwise, we
  will report 1 less than the correct value.
  
  As part of this fix, also update the MPASS assertions to match the real
  expectations. First, ensure that prev is not equal to cur, since this
  should never happen. Second, remove the assertion about prev==0 || delta
  != 0. It looks like that originated from when the em driver was
  converted to iflib. It seems like it was supposed to ensure that delta
  was non-zero. However, because we originally returned 0 delta for the
  first calculation, the "prev == 0" was tacked on.
  
  Instead, replace this with a check that delta is greater than zero,
  after the correction necessary when the ring pointers wrap around.
  
  This new solution should fix the same bug as r341156 did, but in a more
  robust way.
  
  Submitted by:	Jacob Keller <jacob.e.keller@intel.com>
  Reviewed by:	shurd@
  Differential Revision:	https://reviews.freebsd.org/D18545

Modified:
  stable/12/sys/dev/e1000/em_txrx.c
  stable/12/sys/dev/e1000/if_em.c
  stable/12/sys/dev/e1000/igb_txrx.c
  stable/12/sys/dev/ixgbe/if_ix.c
  stable/12/sys/dev/ixgbe/if_ixv.c
  stable/12/sys/dev/ixgbe/ix_txrx.c
  stable/12/sys/dev/ixl/ixl_txrx.c
Directory Properties:
  stable/12/   (props changed)

Modified: stable/12/sys/dev/e1000/em_txrx.c
==============================================================================
--- stable/12/sys/dev/e1000/em_txrx.c	Wed Feb 13 14:32:14 2019	(r344096)
+++ stable/12/sys/dev/e1000/em_txrx.c	Wed Feb 13 14:36:24 2019	(r344097)
@@ -457,16 +457,11 @@ em_isc_txd_credits_update(void *arg, uint16_t txqid, b
 	prev = txr->tx_cidx_processed;
 	ntxd = scctx->isc_ntxd[0];
 	do {
+		MPASS(prev != cur);
 		delta = (int32_t)cur - (int32_t)prev;
-		/*
-		 * XXX This appears to be a hack for first-packet.
-		 * A correct fix would prevent prev == cur in the first place.
-		 */
-		MPASS(prev == 0 || delta != 0);
-		if (prev == 0 && cur == 0)
-			delta += 1;
 		if (delta < 0)
 			delta += ntxd;
+		MPASS(delta > 0);
 		DPRINTF(iflib_get_dev(adapter->ctx),
 			      "%s: cidx_processed=%u cur=%u clear=%d delta=%d\n",
 			      __FUNCTION__, prev, cur, clear, delta);

Modified: stable/12/sys/dev/e1000/if_em.c
==============================================================================
--- stable/12/sys/dev/e1000/if_em.c	Wed Feb 13 14:32:14 2019	(r344096)
+++ stable/12/sys/dev/e1000/if_em.c	Wed Feb 13 14:36:24 2019	(r344097)
@@ -1210,6 +1210,7 @@ static void
 em_if_init(if_ctx_t ctx)
 {
 	struct adapter *adapter = iflib_get_softc(ctx);
+	if_softc_ctx_t scctx = adapter->shared;
 	struct ifnet *ifp = iflib_get_ifp(ctx);
 	struct em_tx_queue *tx_que;
 	int i;
@@ -1242,7 +1243,14 @@ em_if_init(if_ctx_t ctx)
 	for (i = 0, tx_que = adapter->tx_queues; i < adapter->tx_num_queues; i++, tx_que++) {
 		struct tx_ring *txr = &tx_que->txr;
 
-		txr->tx_rs_cidx = txr->tx_rs_pidx = txr->tx_cidx_processed = 0;
+		txr->tx_rs_cidx = txr->tx_rs_pidx;
+
+		/* Initialize the last processed descriptor to be the end of
+		 * the ring, rather than the start, so that we avoid an
+		 * off-by-one error when calculating how many descriptors are
+		 * done in the credits_update function.
+		 */
+		txr->tx_cidx_processed = scctx->isc_ntxd[0] - 1;
 	}
 
 	/* Setup VLAN support, basic and offload if available */

Modified: stable/12/sys/dev/e1000/igb_txrx.c
==============================================================================
--- stable/12/sys/dev/e1000/igb_txrx.c	Wed Feb 13 14:32:14 2019	(r344096)
+++ stable/12/sys/dev/e1000/igb_txrx.c	Wed Feb 13 14:36:24 2019	(r344097)
@@ -332,16 +332,11 @@ igb_isc_txd_credits_update(void *arg, uint16_t txqid, 
 	prev = txr->tx_cidx_processed;
 	ntxd = scctx->isc_ntxd[0];
 	do {
+		MPASS(prev != cur);
 		delta = (int32_t)cur - (int32_t)prev;
-		/*
-		 * XXX This appears to be a hack for first-packet.
-		 * A correct fix would prevent prev == cur in the first place.
-		 */
-		MPASS(prev == 0 || delta != 0);
-		if (prev == 0 && cur == 0)
-			delta += 1;
 		if (delta < 0)
 			delta += ntxd;
+		MPASS(delta > 0);
 
 		processed += delta;
 		prev  = cur;

Modified: stable/12/sys/dev/ixgbe/if_ix.c
==============================================================================
--- stable/12/sys/dev/ixgbe/if_ix.c	Wed Feb 13 14:32:14 2019	(r344096)
+++ stable/12/sys/dev/ixgbe/if_ix.c	Wed Feb 13 14:36:24 2019	(r344097)
@@ -806,7 +806,8 @@ ixgbe_initialize_transmit_units(if_ctx_t ctx)
 		IXGBE_WRITE_REG(hw, IXGBE_TDT(j), 0);
 
 		/* Cache the tail address */
-		txr->tx_rs_cidx = txr->tx_rs_pidx = txr->tx_cidx_processed = 0;
+		txr->tx_rs_cidx = txr->tx_rs_pidx;
+		txr->tx_cidx_processed = scctx->isc_ntxd[0] - 1;
 		for (int k = 0; k < scctx->isc_ntxd[0]; k++)
 			txr->tx_rsq[k] = QIDX_INVALID;
 

Modified: stable/12/sys/dev/ixgbe/if_ixv.c
==============================================================================
--- stable/12/sys/dev/ixgbe/if_ixv.c	Wed Feb 13 14:32:14 2019	(r344096)
+++ stable/12/sys/dev/ixgbe/if_ixv.c	Wed Feb 13 14:36:24 2019	(r344097)
@@ -1227,7 +1227,13 @@ ixv_initialize_transmit_units(if_ctx_t ctx)
 		/* Set Tx Tail register */
 		txr->tail = IXGBE_VFTDT(j);
 
-		txr->tx_rs_cidx = txr->tx_rs_pidx = txr->tx_cidx_processed = 0;
+		txr->tx_rs_cidx = txr->tx_rs_pidx;
+		/* Initialize the last processed descriptor to be the end of
+		 * the ring, rather than the start, so that we avoid an
+		 * off-by-one error when calculating how many descriptors are
+		 * done in the credits_update function.
+		 */
+		txr->tx_cidx_processed = scctx->isc_ntxd[0] - 1;
 		for (int k = 0; k < scctx->isc_ntxd[0]; k++)
 			txr->tx_rsq[k] = QIDX_INVALID;
 

Modified: stable/12/sys/dev/ixgbe/ix_txrx.c
==============================================================================
--- stable/12/sys/dev/ixgbe/ix_txrx.c	Wed Feb 13 14:32:14 2019	(r344096)
+++ stable/12/sys/dev/ixgbe/ix_txrx.c	Wed Feb 13 14:36:24 2019	(r344097)
@@ -296,11 +296,11 @@ ixgbe_isc_txd_credits_update(void *arg, uint16_t txqid
 	prev = txr->tx_cidx_processed;
 	ntxd = scctx->isc_ntxd[0];
 	do {
+		MPASS(prev != cur);
 		delta = (int32_t)cur - (int32_t)prev;
-		if (prev == 0 && cur == 0)
-			delta += 1;
 		if (delta < 0)
 			delta += ntxd;
+		MPASS(delta > 0);
 
 		processed += delta;
 		prev = cur;

Modified: stable/12/sys/dev/ixl/ixl_txrx.c
==============================================================================
--- stable/12/sys/dev/ixl/ixl_txrx.c	Wed Feb 13 14:32:14 2019	(r344096)
+++ stable/12/sys/dev/ixl/ixl_txrx.c	Wed Feb 13 14:36:24 2019	(r344097)
@@ -515,16 +515,11 @@ ixl_isc_txd_credits_update_dwb(void *arg, uint16_t txq
 	prev = txr->tx_cidx_processed;
 	ntxd = scctx->isc_ntxd[0];
 	do {
+		MPASS(prev != cur);
 		delta = (int32_t)cur - (int32_t)prev;
-		/*
-		 * XXX This appears to be a hack for first-packet.
-		 * A correct fix would prevent prev == cur in the first place.
-		 */
-		MPASS(prev == 0 || delta != 0);
-		if (prev == 0 && cur == 0)
-			delta += 1;
 		if (delta < 0)
 			delta += ntxd;
+		MPASS(delta > 0);
 #if 0
 		device_printf(iflib_get_dev(vsi->ctx),
 			      "%s: (q%d) cidx_processed=%u cur=%u clear=%d delta=%d\n",
@@ -793,8 +788,15 @@ ixl_init_tx_rsqs(struct ixl_vsi *vsi)
 	for (i = 0, tx_que = vsi->tx_queues; i < vsi->num_tx_queues; i++, tx_que++) {
 		struct tx_ring *txr = &tx_que->txr;
 
-		txr->tx_rs_cidx = txr->tx_rs_pidx = txr->tx_cidx_processed = 0;
+		txr->tx_rs_cidx = txr->tx_rs_pidx;
 
+		/* Initialize the last processed descriptor to be the end of
+		 * the ring, rather than the start, so that we avoid an
+		 * off-by-one error when calculating how many descriptors are
+		 * done in the credits_update function.
+		 */
+		txr->tx_cidx_processed = scctx->isc_ntxd[0] - 1;
+
 		for (j = 0; j < scctx->isc_ntxd[0]; j++)
 			txr->tx_rsq[j] = QIDX_INVALID;
 	}
@@ -803,13 +805,14 @@ ixl_init_tx_rsqs(struct ixl_vsi *vsi)
 void
 ixl_init_tx_cidx(struct ixl_vsi *vsi)
 {
+	if_softc_ctx_t scctx = vsi->shared;
 	struct ixl_tx_queue *tx_que;
 	int i;
 	
 	for (i = 0, tx_que = vsi->tx_queues; i < vsi->num_tx_queues; i++, tx_que++) {
 		struct tx_ring *txr = &tx_que->txr;
 
-		txr->tx_cidx_processed = 0;
+		txr->tx_cidx_processed = scctx->isc_ntxd[0] - 1;
 	}
 }
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201902131436.x1DEaOM7040710>