I was forced to revisit iflib's transmit cleaning yesterday while looking at a bug that mav@ was hitting when doing a dd over iscsi that would cause regular stalls. This is a longstanding bug that I'm pretty certain is my fault. However, it should have been self evident if the intent of the code were clear. So apart from fixing the bug itself a bigger question that I have for reviewers is how best to go about durably communicating the design intentions.
The evolution of an optimization to minimize doorbell register writes and an adaptation to cope with llimited msi-x vectors on some hardware have combined to create a particularly insidious bug that has lingered for several years.
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 have 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.
for (int i = 0; i < adapter->num_rx_queues; i++, vector++, rx_que++) {
rid = vector + 1;
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);
<snip>
}
for (int i = 0; i < adapter->num_tx_queues; i++) {
snprintf(buf, sizeof(buf), "txq%d", i);
tx_que = &adapter->tx_queues[i];
tx_que->msix = i % adapter->num_rx_queues;
iflib_softirq_alloc_generic(ctx,
&adapter->rx_queues[tx_que->msix].que_irq,
IFLIB_INTR_TX, tx_que, tx_que->txr.me, buf);
}There's nothing other than the periodic iflib timer driving this tx task by default.
So when sharing a vector the first part needs to be:
for (int i = 0; i < adapter->num_rx_queues; i++, vector++, rx_que++) {
rid = vector + 1;
snprintf(buf, sizeof(buf), "rxq%d", i);
error = iflib_irq_alloc_generic(ctx, &rx_que->que_irq, rid,
IFLIB_INTR_RXTX, ixgbe_msix_que, rx_que, rx_que->rxr.me, buf);
<snip>Currently this requires a 1:1 mapping between rx queues and tx queues because of this code:
for (rxconf = i = 0; i < nrxqsets; i++, rxconf++, rxq++) {
/* Set up some basics */
callout_init(&rxq->ifr_watchdog, 1);
if ((ifdip = malloc(sizeof(struct iflib_dma_info) * nrxqs,
M_IFLIB, M_NOWAIT | M_ZERO)) == NULL) {
device_printf(dev,
"Unable to allocate RX DMA info memory\n");
err = ENOMEM;
goto err_tx_desc;
}
rxq->ifr_ifdi = ifdip;
/* XXX this needs to be changed if #rx queues != #tx queues */
rxq->ifr_ntxqirq = 1;
rxq->ifr_txqid[0] = i;If someone wants to setup #txq > #rxq we'll need to add a programmatic interface to change that value. This should be fairly easy to implement, but coordinating testing would be rather onerous.