From owner-svn-src-all@freebsd.org Wed Feb 13 14:36:28 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 46A7D14F113B; Wed, 13 Feb 2019 14:36:28 +0000 (UTC) (envelope-from marius@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 E252C8C80C; Wed, 13 Feb 2019 14:36:27 +0000 (UTC) (envelope-from marius@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 393F9288DE; Wed, 13 Feb 2019 14:36:26 +0000 (UTC) (envelope-from marius@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id x1DEaQAT040716; Wed, 13 Feb 2019 14:36:26 GMT (envelope-from marius@FreeBSD.org) Received: (from marius@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id x1DEaOM7040710; Wed, 13 Feb 2019 14:36:24 GMT (envelope-from marius@FreeBSD.org) Message-Id: <201902131436.x1DEaOM7040710@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: marius set sender to marius@FreeBSD.org using -f From: Marius Strobl Date: Wed, 13 Feb 2019 14:36:24 +0000 (UTC) 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 X-SVN-Group: stable-12 X-SVN-Commit-Author: marius X-SVN-Commit-Paths: in stable/12/sys/dev: e1000 ixgbe ixl X-SVN-Commit-Revision: 344097 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: E252C8C80C X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.95 / 15.00]; local_wl_from(0.00)[FreeBSD.org]; NEURAL_HAM_MEDIUM(-0.99)[-0.995,0]; NEURAL_HAM_SHORT(-0.96)[-0.958,0]; ASN(0.00)[asn:11403, ipnet:2610:1c1:1::/48, country:US]; NEURAL_HAM_LONG(-1.00)[-0.999,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: Wed, 13 Feb 2019 14:36:28 -0000 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 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; } }