Date: Thu, 7 Jan 2021 22:08:47 GMT From: Matt Macy <mmacy@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 81be655266fa - main - iflib: ensure that tx interrupts enabled and cleanups Message-ID: <202101072208.107M8llR063703@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by mmacy: URL: https://cgit.FreeBSD.org/src/commit/?id=81be655266fac2b333e25f386f32c9bcd17f523d commit 81be655266fac2b333e25f386f32c9bcd17f523d Author: Matt Macy <mmacy@FreeBSD.org> AuthorDate: 2020-12-19 01:08:33 +0000 Commit: Matt Macy <mmacy@FreeBSD.org> CommitDate: 2021-01-07 22:07:35 +0000 iflib: ensure that tx interrupts enabled and cleanups Doing a 'dd' over iscsi will reliably cause stalls. Tx cleaning _should_ reliably happen as data is sent. However, currently if the transmit queue fills it will wait until the iflib timer (hz/2) runs. This change causes the the tx taskq thread to be run if there are completed descriptors. While here: - make timer interrupt delay a sysctl - simplify txd_db_check handling - comment on INTR types Background on the change: Initially doorbell updates were minimized by only writing to the register on every fourth packet. If txq_drain would return without writing to the doorbell it scheduled a callout on the next tick to do the doorbell write to ensure that the write otherwise happened "soon". At that time a sysctl was added for users to avoid the potential added latency by simply writing to the doorbell register on every packet. This worked perfectly well for e1000 and ixgbe ... and appeared to work well on ixl. However, as it turned out there was a race to this approach that would lockup the ixl MAC. It was possible for a lower producer index to be written after a higher one. On e1000 and ixgbe this was harmless - on ixl it was fatal. My initial response was to add a lock around doorbell writes - fixing the problem but adding an unacceptable amount of lock contention. The next iteration was to use transmit interrupts to drive delayed doorbell writes. If there were no packets in the queue all doorbell writes would be immediate as the queue started to fill up we could delay doorbell writes further and further. At the start of drain if we've cleaned any packets we know we've moved the state machine along and we write the doorbell (an obvious missing optimization was to skip that doorbell write if db_pending is zero). This change required that tx interrupts be scheduled periodically as opposed to just when the hardware txq was full. However, that just leads to our next problem. Initially dedicated msix vectors were used for both tx and rx. However, it was often possible to use up all available vectors before we set up all the queues we wanted. By having rx and tx share a vector for a given queue we could halve the number of vectors used by a given configuration. The problem here is that with this change only e1000 passed the necessary value to have the fast interrupt drive tx when appropriate. Reported by: mav@ Tested by: mav@ Reviewed by: gallatin@ MFC after: 1 month Sponsored by: iXsystems Differential Revision: https://reviews.freebsd.org/D27683 --- sys/dev/axgbe/if_axgbe_pci.c | 2 +- sys/dev/bnxt/if_bnxt.c | 2 +- sys/dev/ice/if_ice_iflib.c | 2 +- sys/dev/ixgbe/if_ix.c | 2 +- sys/dev/ixgbe/if_ixv.c | 2 +- sys/dev/ixl/if_iavf.c | 2 +- sys/dev/ixl/if_ixl.c | 2 +- sys/dev/mgb/if_mgb.c | 2 +- sys/dev/vmware/vmxnet3/if_vmx.c | 2 +- sys/net/iflib.c | 94 +++++++++++++++++++++++++++-------------- sys/net/iflib.h | 17 ++++++++ 11 files changed, 88 insertions(+), 41 deletions(-) diff --git a/sys/dev/axgbe/if_axgbe_pci.c b/sys/dev/axgbe/if_axgbe_pci.c index 4c4883e1cb4f..7c4645792321 100644 --- a/sys/dev/axgbe/if_axgbe_pci.c +++ b/sys/dev/axgbe/if_axgbe_pci.c @@ -1922,7 +1922,7 @@ axgbe_if_msix_intr_assign(if_ctx_t ctx, int msix) channel = pdata->channel[i]; snprintf(buf, sizeof(buf), "rxq%d", i); - error = iflib_irq_alloc_generic(ctx, &irq, rid, IFLIB_INTR_RX, + error = iflib_irq_alloc_generic(ctx, &irq, rid, IFLIB_INTR_RXTX, axgbe_msix_que, channel, channel->queue_index, buf); if (error) { diff --git a/sys/dev/bnxt/if_bnxt.c b/sys/dev/bnxt/if_bnxt.c index 724fd0469ef5..7811f4fdebf0 100644 --- a/sys/dev/bnxt/if_bnxt.c +++ b/sys/dev/bnxt/if_bnxt.c @@ -1534,7 +1534,7 @@ bnxt_msix_intr_assign(if_ctx_t ctx, int msix) for (i=0; i<softc->scctx->isc_nrxqsets; i++) { snprintf(irq_name, sizeof(irq_name), "rxq%d", i); rc = iflib_irq_alloc_generic(ctx, &softc->rx_cp_rings[i].irq, - softc->rx_cp_rings[i].ring.id + 1, IFLIB_INTR_RX, + softc->rx_cp_rings[i].ring.id + 1, IFLIB_INTR_RXTX, bnxt_handle_rx_cp, &softc->rx_cp_rings[i], i, irq_name); if (rc) { device_printf(iflib_get_dev(ctx), diff --git a/sys/dev/ice/if_ice_iflib.c b/sys/dev/ice/if_ice_iflib.c index dffdbc391f78..a3004b8f6c8d 100644 --- a/sys/dev/ice/if_ice_iflib.c +++ b/sys/dev/ice/if_ice_iflib.c @@ -1493,7 +1493,7 @@ ice_if_msix_intr_assign(if_ctx_t ctx, int msix) snprintf(irq_name, sizeof(irq_name), "rxq%d", i); err = iflib_irq_alloc_generic(ctx, &sc->irqvs[vector].irq, rid, - IFLIB_INTR_RX, ice_msix_que, + IFLIB_INTR_RXTX, ice_msix_que, rxq, rxq->me, irq_name); if (err) { device_printf(sc->dev, diff --git a/sys/dev/ixgbe/if_ix.c b/sys/dev/ixgbe/if_ix.c index 783c50d5a3cc..7eb32077f8b0 100644 --- a/sys/dev/ixgbe/if_ix.c +++ b/sys/dev/ixgbe/if_ix.c @@ -2026,7 +2026,7 @@ ixgbe_if_msix_intr_assign(if_ctx_t ctx, int msix) snprintf(buf, sizeof(buf), "rxq%d", i); error = iflib_irq_alloc_generic(ctx, &rx_que->que_irq, rid, - IFLIB_INTR_RX, ixgbe_msix_que, rx_que, rx_que->rxr.me, buf); + IFLIB_INTR_RXTX, ixgbe_msix_que, rx_que, rx_que->rxr.me, buf); if (error) { device_printf(iflib_get_dev(ctx), diff --git a/sys/dev/ixgbe/if_ixv.c b/sys/dev/ixgbe/if_ixv.c index 7b8c7f169a7e..6bd92d262558 100644 --- a/sys/dev/ixgbe/if_ixv.c +++ b/sys/dev/ixgbe/if_ixv.c @@ -1030,7 +1030,7 @@ ixv_if_msix_intr_assign(if_ctx_t ctx, int msix) snprintf(buf, sizeof(buf), "rxq%d", i); error = iflib_irq_alloc_generic(ctx, &rx_que->que_irq, rid, - IFLIB_INTR_RX, ixv_msix_que, rx_que, rx_que->rxr.me, buf); + IFLIB_INTR_RXTX, ixv_msix_que, rx_que, rx_que->rxr.me, buf); if (error) { device_printf(iflib_get_dev(ctx), diff --git a/sys/dev/ixl/if_iavf.c b/sys/dev/ixl/if_iavf.c index d85cb8ac0541..2079e2d7306b 100644 --- a/sys/dev/ixl/if_iavf.c +++ b/sys/dev/ixl/if_iavf.c @@ -904,7 +904,7 @@ iavf_if_msix_intr_assign(if_ctx_t ctx, int msix) snprintf(buf, sizeof(buf), "rxq%d", i); err = iflib_irq_alloc_generic(ctx, &rx_que->que_irq, rid, - IFLIB_INTR_RX, iavf_msix_que, rx_que, rx_que->rxr.me, buf); + IFLIB_INTR_RXTX, iavf_msix_que, rx_que, rx_que->rxr.me, buf); /* XXX: Does the driver work as expected if there are fewer num_rx_queues than * what's expected in the iflib context? */ if (err) { diff --git a/sys/dev/ixl/if_ixl.c b/sys/dev/ixl/if_ixl.c index 4d953c45ab91..50eb448a1154 100644 --- a/sys/dev/ixl/if_ixl.c +++ b/sys/dev/ixl/if_ixl.c @@ -1051,7 +1051,7 @@ ixl_if_msix_intr_assign(if_ctx_t ctx, int msix) snprintf(buf, sizeof(buf), "rxq%d", i); err = iflib_irq_alloc_generic(ctx, &rx_que->que_irq, rid, - IFLIB_INTR_RX, ixl_msix_que, rx_que, rx_que->rxr.me, buf); + IFLIB_INTR_RXTX, ixl_msix_que, rx_que, rx_que->rxr.me, buf); /* XXX: Does the driver work as expected if there are fewer num_rx_queues than * what's expected in the iflib context? */ if (err) { diff --git a/sys/dev/mgb/if_mgb.c b/sys/dev/mgb/if_mgb.c index ce4d43b8d34c..3f7a9d1a9dbb 100644 --- a/sys/dev/mgb/if_mgb.c +++ b/sys/dev/mgb/if_mgb.c @@ -855,7 +855,7 @@ mgb_msix_intr_assign(if_ctx_t ctx, int msix) vectorid++; snprintf(irq_name, sizeof(irq_name), "rxq%d", i); error = iflib_irq_alloc_generic(ctx, &sc->rx_irq, vectorid + 1, - IFLIB_INTR_RX, mgb_rxq_intr, sc, i, irq_name); + IFLIB_INTR_RXTX, mgb_rxq_intr, sc, i, irq_name); if (error) { device_printf(sc->dev, "Failed to register rxq %d interrupt handler\n", i); diff --git a/sys/dev/vmware/vmxnet3/if_vmx.c b/sys/dev/vmware/vmxnet3/if_vmx.c index c2ae55532ed0..340fe1a7dd80 100644 --- a/sys/dev/vmware/vmxnet3/if_vmx.c +++ b/sys/dev/vmware/vmxnet3/if_vmx.c @@ -480,7 +480,7 @@ vmxnet3_msix_intr_assign(if_ctx_t ctx, int msix) rxq = &sc->vmx_rxq[i]; error = iflib_irq_alloc_generic(ctx, &rxq->vxrxq_irq, i + 1, - IFLIB_INTR_RX, vmxnet3_rxq_intr, rxq, i, irq_name); + IFLIB_INTR_RXTX, vmxnet3_rxq_intr, rxq, i, irq_name); if (error) { device_printf(iflib_get_dev(ctx), "Failed to register rxq %d interrupt handler\n", i); diff --git a/sys/net/iflib.c b/sys/net/iflib.c index 5db925fef05f..cc401a5db24d 100644 --- a/sys/net/iflib.c +++ b/sys/net/iflib.c @@ -585,6 +585,10 @@ SYSCTL_INT(_net_iflib, OID_AUTO, min_tx_latency, CTLFLAG_RW, static int iflib_no_tx_batch = 0; SYSCTL_INT(_net_iflib, OID_AUTO, no_tx_batch, CTLFLAG_RW, &iflib_no_tx_batch, 0, "minimize transmit latency at the possible expense of throughput"); +static int iflib_timer_default = 1000; +SYSCTL_INT(_net_iflib, OID_AUTO, timer_default, CTLFLAG_RW, + &iflib_timer_default, 0, "number of ticks between iflib_timer calls"); + #if IFLIB_DEBUG_COUNTERS @@ -2293,7 +2297,7 @@ iflib_timer(void *arg) ** can be done without the lock because its RO ** and the HUNG state will be static if set. */ - if (this_tick - txq->ift_last_timer_tick >= hz / 2) { + if (this_tick - txq->ift_last_timer_tick >= iflib_timer_default) { txq->ift_last_timer_tick = this_tick; IFDI_TIMER(ctx, txq->ift_id); if ((txq->ift_qstatus == IFLIB_QUEUE_HUNG) && @@ -2303,7 +2307,8 @@ iflib_timer(void *arg) if (txq->ift_qstatus != IFLIB_QUEUE_IDLE && ifmp_ring_is_stalled(txq->ift_br)) { - KASSERT(ctx->ifc_link_state == LINK_STATE_UP, ("queue can't be marked as hung if interface is down")); + KASSERT(ctx->ifc_link_state == LINK_STATE_UP, + ("queue can't be marked as hung if interface is down")); txq->ift_qstatus = IFLIB_QUEUE_HUNG; } txq->ift_cleaned_prev = txq->ift_cleaned; @@ -2314,7 +2319,8 @@ iflib_timer(void *arg) sctx->isc_pause_frames = 0; if (if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_RUNNING) - callout_reset_on(&txq->ift_timer, hz / 2, iflib_timer, txq, txq->ift_timer.c_cpu); + callout_reset_on(&txq->ift_timer, iflib_timer_default, iflib_timer, + txq, txq->ift_timer.c_cpu); return; hung: @@ -2426,7 +2432,7 @@ done: IFDI_INTR_ENABLE(ctx); txq = ctx->ifc_txqs; for (i = 0; i < sctx->isc_ntxqsets; i++, txq++) - callout_reset_on(&txq->ift_timer, hz/2, iflib_timer, txq, + callout_reset_on(&txq->ift_timer, iflib_timer_default, iflib_timer, txq, txq->ift_timer.c_cpu); } @@ -3001,26 +3007,37 @@ txq_max_rs_deferred(iflib_txq_t txq) /* XXX we should be setting this to something other than zero */ #define RECLAIM_THRESH(ctx) ((ctx)->ifc_sctx->isc_tx_reclaim_thresh) -#define MAX_TX_DESC(ctx) max((ctx)->ifc_softc_ctx.isc_tx_tso_segments_max, \ +#define MAX_TX_DESC(ctx) MAX((ctx)->ifc_softc_ctx.isc_tx_tso_segments_max, \ (ctx)->ifc_softc_ctx.isc_tx_nsegments) static inline bool -iflib_txd_db_check(if_ctx_t ctx, iflib_txq_t txq, int ring, qidx_t in_use) +iflib_txd_db_check(iflib_txq_t txq, int ring) { + if_ctx_t ctx = txq->ift_ctx; qidx_t dbval, max; - bool rang; - rang = false; - max = TXQ_MAX_DB_DEFERRED(txq, in_use); - if (ring || txq->ift_db_pending >= max) { + max = TXQ_MAX_DB_DEFERRED(txq, txq->ift_in_use); + + /* force || threshold exceeded || at the edge of the ring */ + if (ring || (txq->ift_db_pending >= max) || (TXQ_AVAIL(txq) <= MAX_TX_DESC(ctx) + 2)) { + + /* + * 'npending' is used if the card's doorbell is in terms of the number of descriptors + * pending flush (BRCM). 'pidx' is used in cases where the card's doorbeel uses the + * producer index explicitly (INTC). + */ dbval = txq->ift_npending ? txq->ift_npending : txq->ift_pidx; bus_dmamap_sync(txq->ift_ifdi->idi_tag, txq->ift_ifdi->idi_map, BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE); ctx->isc_txd_flush(ctx->ifc_softc, txq->ift_id, dbval); + + /* + * Absent bugs there are zero packets pending so reset pending counts to zero. + */ txq->ift_db_pending = txq->ift_npending = 0; - rang = true; + return (true); } - return (rang); + return (false); } #ifdef PKT_DEBUG @@ -3481,6 +3498,7 @@ defrag: MPASS(pi.ipi_new_pidx != pidx); MPASS(ndesc > 0); txq->ift_in_use += ndesc; + txq->ift_db_pending += ndesc; /* * We update the last software descriptor again here because there may @@ -3647,8 +3665,8 @@ iflib_txq_drain(struct ifmp_ring *r, uint32_t cidx, uint32_t pidx) if_ctx_t ctx = txq->ift_ctx; if_t ifp = ctx->ifc_ifp; struct mbuf *m, **mp; - int avail, bytes_sent, consumed, count, err, i, in_use_prev; - int mcast_sent, pkt_sent, reclaimed, txq_avail; + int avail, bytes_sent, skipped, count, err, i; + int mcast_sent, pkt_sent, reclaimed; bool do_prefetch, rang, ring; if (__predict_false(!(if_getdrvflags(ifp) & IFF_DRV_RUNNING) || @@ -3657,9 +3675,13 @@ iflib_txq_drain(struct ifmp_ring *r, uint32_t cidx, uint32_t pidx) return (0); } reclaimed = iflib_completed_tx_reclaim(txq, RECLAIM_THRESH(ctx)); - rang = iflib_txd_db_check(ctx, txq, reclaimed, txq->ift_in_use); + rang = iflib_txd_db_check(txq, reclaimed && txq->ift_db_pending); avail = IDXDIFF(pidx, cidx, r->size); + if (__predict_false(ctx->ifc_flags & IFC_QFLUSH)) { + /* + * The driver is unloading so we need to free all pending packets. + */ DBG_COUNTER_INC(txq_drain_flushing); for (i = 0; i < avail; i++) { if (__predict_true(r->items[(cidx + i) & (r->size-1)] != (void *)txq)) @@ -3677,9 +3699,13 @@ iflib_txq_drain(struct ifmp_ring *r, uint32_t cidx, uint32_t pidx) DBG_COUNTER_INC(txq_drain_oactive); return (0); } + + /* + * If we've reclaimed any packets this queue cannot be hung. + */ if (reclaimed) txq->ift_qstatus = IFLIB_QUEUE_IDLE; - consumed = mcast_sent = bytes_sent = pkt_sent = 0; + skipped = mcast_sent = bytes_sent = pkt_sent = 0; count = MIN(avail, TX_BATCH_SIZE); #ifdef INVARIANTS if (iflib_verbose_debug) @@ -3687,54 +3713,57 @@ iflib_txq_drain(struct ifmp_ring *r, uint32_t cidx, uint32_t pidx) avail, ctx->ifc_flags, TXQ_AVAIL(txq)); #endif do_prefetch = (ctx->ifc_flags & IFC_PREFETCH); - txq_avail = TXQ_AVAIL(txq); err = 0; - for (i = 0; i < count && txq_avail > MAX_TX_DESC(ctx) + 2; i++) { + for (i = 0; i < count && TXQ_AVAIL(txq) >= MAX_TX_DESC(ctx) + 2; i++) { int rem = do_prefetch ? count - i : 0; mp = _ring_peek_one(r, cidx, i, rem); MPASS(mp != NULL && *mp != NULL); + + /* + * Completion interrupts will use the address of the txq + * as a sentinel to enqueue _something_ in order to acquire + * the lock on the mp_ring (there's no direct lock call). + * We obviously whave to check for these sentinel cases + * and skip them. + */ if (__predict_false(*mp == (struct mbuf *)txq)) { - consumed++; + skipped++; continue; } - in_use_prev = txq->ift_in_use; err = iflib_encap(txq, mp); if (__predict_false(err)) { /* no room - bail out */ if (err == ENOBUFS) break; - consumed++; + skipped++; /* we can't send this packet - skip it */ continue; } - consumed++; pkt_sent++; m = *mp; DBG_COUNTER_INC(tx_sent); bytes_sent += m->m_pkthdr.len; mcast_sent += !!(m->m_flags & M_MCAST); - txq_avail = TXQ_AVAIL(txq); - txq->ift_db_pending += (txq->ift_in_use - in_use_prev); - ETHER_BPF_MTAP(ifp, m); if (__predict_false(!(ifp->if_drv_flags & IFF_DRV_RUNNING))) break; - rang = iflib_txd_db_check(ctx, txq, false, in_use_prev); + ETHER_BPF_MTAP(ifp, m); + rang = iflib_txd_db_check(txq, false); } /* deliberate use of bitwise or to avoid gratuitous short-circuit */ - ring = rang ? false : (iflib_min_tx_latency | err) || (TXQ_AVAIL(txq) < MAX_TX_DESC(ctx)); - iflib_txd_db_check(ctx, txq, ring, txq->ift_in_use); + ring = rang ? false : (iflib_min_tx_latency | err); + iflib_txd_db_check(txq, ring); if_inc_counter(ifp, IFCOUNTER_OBYTES, bytes_sent); if_inc_counter(ifp, IFCOUNTER_OPACKETS, pkt_sent); if (mcast_sent) if_inc_counter(ifp, IFCOUNTER_OMCASTS, mcast_sent); #ifdef INVARIANTS if (iflib_verbose_debug) - printf("consumed=%d\n", consumed); + printf("consumed=%d\n", skipped + pkt_sent); #endif - return (consumed); + return (skipped + pkt_sent); } static uint32_t @@ -3907,7 +3936,7 @@ _task_fn_admin(void *context) } IFDI_UPDATE_ADMIN_STATUS(ctx); for (txq = ctx->ifc_txqs, i = 0; i < sctx->isc_ntxqsets; i++, txq++) { - callout_reset_on(&txq->ift_timer, hz / 2, iflib_timer, txq, + callout_reset_on(&txq->ift_timer, iflib_timer_default, iflib_timer, txq, txq->ift_timer.c_cpu); } IFDI_LINK_INTR_ENABLE(ctx); @@ -5301,6 +5330,7 @@ iflib_device_iov_add_vf(device_t dev, uint16_t vfnum, const nvlist_t *params) static int iflib_module_init(void) { + iflib_timer_default = hz / 2; return (0); } @@ -6861,7 +6891,7 @@ iflib_debugnet_transmit(if_t ifp, struct mbuf *m) txq = &ctx->ifc_txqs[0]; error = iflib_encap(txq, &m); if (error == 0) - (void)iflib_txd_db_check(ctx, txq, true, txq->ift_in_use); + (void)iflib_txd_db_check(txq, true); return (error); } diff --git a/sys/net/iflib.h b/sys/net/iflib.h index eaf8ea33355d..a30740e67b6e 100644 --- a/sys/net/iflib.h +++ b/sys/net/iflib.h @@ -288,10 +288,27 @@ typedef struct iflib_dma_info { #define IFLIB_MAGIC 0xCAFEF00D typedef enum { + /* Interrupt or softirq handles only receive */ IFLIB_INTR_RX, + + /* Interrupt or softirq handles only transmit */ IFLIB_INTR_TX, + + /* + * Interrupt will check for both pending receive + * and available tx credits and dispatch a task + * for one or both depending on the disposition + * of the respective queues. + */ IFLIB_INTR_RXTX, + + /* + * Other interrupt - typically link status and + * or error conditions. + */ IFLIB_INTR_ADMIN, + + /* Softirq (task) for iov handling */ IFLIB_INTR_IOV, } iflib_intr_type_t;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202101072208.107M8llR063703>