From owner-svn-src-all@freebsd.org Thu Jan 24 01:03:02 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id AEA1514B8059; Thu, 24 Jan 2019 01:03:02 +0000 (UTC) (envelope-from erj@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 51A346DD4A; Thu, 24 Jan 2019 01:03:02 +0000 (UTC) (envelope-from erj@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 42A8D20CC7; Thu, 24 Jan 2019 01:03:02 +0000 (UTC) (envelope-from erj@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id x0O132q1041730; Thu, 24 Jan 2019 01:03:02 GMT (envelope-from erj@FreeBSD.org) Received: (from erj@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id x0O130xi041721; Thu, 24 Jan 2019 01:03:00 GMT (envelope-from erj@FreeBSD.org) Message-Id: <201901240103.x0O130xi041721@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: erj set sender to erj@FreeBSD.org using -f From: Eric Joyner Date: Thu, 24 Jan 2019 01:03:00 +0000 (UTC) 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 X-SVN-Group: head X-SVN-Commit-Author: erj X-SVN-Commit-Paths: in head/sys/dev: e1000 ixgbe ixl X-SVN-Commit-Revision: 343369 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 51A346DD4A X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.97 / 15.00]; local_wl_from(0.00)[FreeBSD.org]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; NEURAL_HAM_SHORT(-0.97)[-0.968,0]; ASN(0.00)[asn:11403, ipnet:2610:1c1:1::/48, country:US]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Jan 2019 01:03:03 -0000 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 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; } }