Date: Thu, 24 Jan 2019 01:03:00 +0000 (UTC) From: Eric Joyner <erj@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r343369 - in head/sys/dev: e1000 ixgbe ixl Message-ID: <201901240103.x0O130xi041721@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: erj Date: Thu Jan 24 01:03:00 2019 New Revision: 343369 URL: https://svnweb.freebsd.org/changeset/base/343369 Log: 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@ Sponsored by: Intel Corporation Differential Revision: https://reviews.freebsd.org/D18545 Modified: head/sys/dev/e1000/em_txrx.c head/sys/dev/e1000/if_em.c head/sys/dev/e1000/igb_txrx.c head/sys/dev/ixgbe/if_ix.c head/sys/dev/ixgbe/if_ixv.c head/sys/dev/ixgbe/ix_txrx.c head/sys/dev/ixl/ixl_txrx.c Modified: head/sys/dev/e1000/em_txrx.c ============================================================================== --- head/sys/dev/e1000/em_txrx.c Wed Jan 23 23:48:57 2019 (r343368) +++ head/sys/dev/e1000/em_txrx.c Thu Jan 24 01:03:00 2019 (r343369) @@ -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: head/sys/dev/e1000/if_em.c ============================================================================== --- head/sys/dev/e1000/if_em.c Wed Jan 23 23:48:57 2019 (r343368) +++ head/sys/dev/e1000/if_em.c Thu Jan 24 01:03:00 2019 (r343369) @@ -1208,6 +1208,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; @@ -1240,7 +1241,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: head/sys/dev/e1000/igb_txrx.c ============================================================================== --- head/sys/dev/e1000/igb_txrx.c Wed Jan 23 23:48:57 2019 (r343368) +++ head/sys/dev/e1000/igb_txrx.c Thu Jan 24 01:03:00 2019 (r343369) @@ -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: head/sys/dev/ixgbe/if_ix.c ============================================================================== --- head/sys/dev/ixgbe/if_ix.c Wed Jan 23 23:48:57 2019 (r343368) +++ head/sys/dev/ixgbe/if_ix.c Thu Jan 24 01:03:00 2019 (r343369) @@ -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: head/sys/dev/ixgbe/if_ixv.c ============================================================================== --- head/sys/dev/ixgbe/if_ixv.c Wed Jan 23 23:48:57 2019 (r343368) +++ head/sys/dev/ixgbe/if_ixv.c Thu Jan 24 01:03:00 2019 (r343369) @@ -1228,7 +1228,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: head/sys/dev/ixgbe/ix_txrx.c ============================================================================== --- head/sys/dev/ixgbe/ix_txrx.c Wed Jan 23 23:48:57 2019 (r343368) +++ head/sys/dev/ixgbe/ix_txrx.c Thu Jan 24 01:03:00 2019 (r343369) @@ -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: head/sys/dev/ixl/ixl_txrx.c ============================================================================== --- head/sys/dev/ixl/ixl_txrx.c Wed Jan 23 23:48:57 2019 (r343368) +++ head/sys/dev/ixl/ixl_txrx.c Thu Jan 24 01:03:00 2019 (r343369) @@ -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?201901240103.x0O130xi041721>