iflib: Ring transmit doorbells once per receive batch
Needs ReviewPublic
Actions

Authored by rcm on Tue, Oct 6, 6:43 PM.

Details

Reviewers

kbowling
gallatin
shurd
tuexen
nickbanks_netflix.com

Group Reviewers

iflib
transport

Summary

With direct dispatch, the receive task transmits each forwarded packet
from within if_input(), and on a lightly used ring every packet rings
the doorbell. 4a0ec469934a stopped iflib from ringing when nothing was
pending, which took three register writes per packet down to one; this
rings the remaining one once per receive batch. On an Intel I226 (igc)
a write costs about 400 cycles, a fifth of the cost of forwarding a
packet with pf disabled.

For the whole of a receive task's pass over its ring, including the LRO
flush, the transmit queues it uses defer their doorbells, and the task
rings each of them once at the end of the pass. The whole pass is
covered, not just the if_input() call after it, because with LRO
enabled and IP forwarding on, tcp_lro_queue_mbuf() hands each packet to
if_input() from inside the receive loop; igc, em, igb, ix and ixl
enable LRO by default. A queue defers only while it is at most an
eighth full, where iflib_txd_db_check() would ring for every packet;
above that iflib already batches on its own. Both the mp_ring and the
simple transmit paths take part. Other transmitters, such as the
transmit task when tx_abdicate is set, ring as before, and nothing
changes when net.iflib.min_tx_latency is set. Batched doorbells are
off by default until they have been exercised on more drivers;
net.iflib.tx_db_batched=1 turns them on, at any time.

The batch lives on the receive task's stack and the task's thread
points at it (td_netbatch), so the transmit path finds it with one load
and a thread that preempts the task does not take part. A queue is
held by one batch at a time, which links it into a list to ring at the
end, and on the mp_ring path the transmit task leaves such descriptors
to the batch: taking over the ring for them would move the draining to
a task that rings per packet, and a backlog builds whenever it falls
behind. mlx5en has held its send queues' doorbells during receive
processing since 2d5e5a0d75b0; DragonFly's ifq staging and OpenBSD's
transmit mitigation batch packets at the ifq layer instead.

Sponsored by: Rubicon Communications, LLC ("Netgate")

Test Plan

On a Netgate 4200 (4 Atom cores, Intel I226) forwarding 64-byte UDP,
one core rings once per 16 packets instead of once per packet and
forwards 1.35 instead of 1.10 Mpps with pf disabled and 0.44 instead of
0.38 Mpps with pf and 198k states; with LRO enabled it forwards 1.31
instead of 1.08 Mpps. With four queues per port it forwards up to
3.5-3.6 instead of 2.6 Mpps one way and 5.4-5.6 instead of 3.9-4.2 Mpps
both ways, on either transmit path. As a TCP endpoint with LRO, it
receives at line rate with one doorbell per 3.5 ACKs instead of one per
ACK, and sends with one per 9 segments instead of nearly one per
segment.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

rcm created this revision.Tue, Oct 6, 6:43 PM

Herald added a reviewer: shurd. · View Herald TranscriptTue, Oct 6, 6:43 PM

Herald added 1 blocking reviewer(s): iflib. · View Herald Transcript

Herald added subscribers: ziaee, glebius, melifaro and 2 others. · View Herald Transcript

rcm requested review of this revision.Tue, Oct 6, 6:43 PM

Start the burst before the receive loop rather than after it, and end it on the error path too, so that packets tcp_lro_queue_mbuf() hands to if_input() from inside the loop are covered; that happens whenever LRO is enabled with IP forwarding on, and igc, em, igb, ix and ixl enable LRO by default.

s/burst/batch/g

no functional change intended

I need to think about this more. For your workload, I see the benefit. For mine (busy CDN server), where we're running TCP, and where our TX rings are constantly > 80% full, I think this just adds overhead (due to full rings) and risk (due to TCP).

TCP is also interesting, because one RX packet can generate megabytes of TX. I'm not sure its appropriate at all for TCP. I'm going to add some folks from the transport side.

Can you make it as cheap as possible to disable? Eg, iflib_txdb_burst() checks iflib_tx_db_burst and returns immediately when its disabled, and doesn't check the pcpu pointer, etc. I can see how that would make it difficult to handle changes to iflib_tx_db_burst , but I'd rather have that as an RDTUN and have this as cheap to disable as possible.

gallatin added reviewers: tuexen, nickbanks_netflix.com, transport.Wed, Oct 7, 3:33 PM

In D60416#1386523, @gallatin wrote:

I need to think about this more. For your workload, I see the benefit. For mine (busy CDN server), where we're running TCP, and where our TX rings are constantly > 80% full, I think this just adds overhead (due to full rings) and risk (due to TCP).

TCP is also interesting, because one RX packet can generate megabytes of TX. I'm not sure its appropriate at all for TCP. I'm going to add some folks from the transport side.

I think your TX rings a full, because they are serving a lot of TCP connections. A single incoming TCP segment belongs to a single TCP connection and only triggers the sending of TCP segments of that TCP connection. A single TCP should not send megabytes of data in response to a single incoming segment. Usually the size of such bursts is limited by a number. Sending a megabytes of data at wire speed might result in packet loss on the way to the destination if some other link is the bandwidth bottleneck.

Can you make it as cheap as possible to disable? Eg, iflib_txdb_burst() checks iflib_tx_db_burst and returns immediately when its disabled, and doesn't check the pcpu pointer, etc. I can see how that would make it difficult to handle changes to iflib_tx_db_burst , but I'd rather have that as an RDTUN and have this as cheap to disable as possible.

luke.hamburg_gmail.com added a subscriber: luke.hamburg_gmail.com.Thu, Oct 8, 12:24 PM

Could we first evaluate tuning txq_max_db_deferred() and having simpletx honor the existing policy?

Please explain what additional flush/progress mechanism that would require, and why a new batch boundary is preferable. That would help establish whether this per-CPU tracking machinery is necessary.

dennypage_me.com added a subscriber: dennypage_me.com.Sun, Oct 11, 4:58 PM

rcm mentioned this in D60756: proc: Add a network transmit batch pointer to struct thread.Thu, Oct 15, 7:46 PM

rcm added a parent revision: D60756: proc: Add a network transmit batch pointer to struct thread.

I have reworked this significantly.

The per-CPU machinery has been dropped. The transmit path now finds the receive pass's batch via curthread->td_netbatch, added to struct thread in D60756.

The disabled path is now one load of the global per drain and a register compare per packet, no per-CPU access. I did leave the sysctl writable. A pass decides once whether to batch and always ends what it began; a transmitter that finds no batch rings as before. I flipped it ~800 times/s over 830M forwarded packets with no stalls and nothing was left unrung. However, I don't have strong opinions either way about the sysctl.

On TCP: a queue is held only while at most 1/8 full and only for one receive pass, so an RX packet that generates megabytes of TX crosses 1/8 within the pass and rings normally from there. As an endpoint with LRO it runs at line rate at one doorbell per 3.5 ACKs in and per 9 segments out.

D60756 was accepted by @kib.

I did explore a few other options like osd(9). Here is my reply to @kib regarding that result:

It is a tad bit slower (~1% w/ pf disabled) based on what I can measure than the thread-local pointer form and it has more failure modes (will need to handle ENOMEM) that will need to be reasoned through. The first set on a thread allocates with M_NOWAIT, so a pass that can't get its slots just won't batch; that seems okay. I did find an issue that probably should be resolved with osd(9) before we consider it as an option. I have posted D60772 that resolves that. The osd(9) form adds something like 50-70 instructions to the hot path. Once pf is enabled though this cost is lost in the noise floor (mostly due to pf stalling on DRAM loads).

The thread-local pointer is simpler and easier to reason about at the cost of growing struct thread.

Did you try measure doing the call to iflib_txdb_batch() in iflib_simple_encap(), rather than adding an argument all the way down? I hate the perceived complexity adding an arg adds.. Pushing it down adds 2 loads from hot cache lines per packet in the case where we have contention and the deferral ring is used, at the human savings of some complexity, and possibly 8 bytes of stack space. I think it won't matter at all for your workload, since I'm guessing your workload is mostly uncontended, so the deferral ring is probably not used.

Also, did you try @kbowling 's suggestion of fixing the db deferral in simple_tx ? I think its just as follows, and I'd be interested to hear how much it helps compared to this complexity:

--- a/sys/net/iflib.c
+++ b/sys/net/iflib.c
@@ -8358,7 +8358,8 @@ iflib_simple_transmit_impl(if_ctx_t ctx, if_t ifp, struct mbuf *m,
 	error = iflib_simple_transmit_locked(txq, m, &bytes_sent, &pkt_sent,
 	    &mcast_sent);
 	if (txq->ift_db_pending != 0)
-		(void)iflib_txd_db_check(txq, true);
+		(void)iflib_txd_db_check(txq, iflib_min_tx_latency | error |
+		    (!!txq->ift_reclaim_thresh));
 	m_defer = NULL;
 	reclaimable = iflib_txq_can_reclaim(txq);
 	if (reclaimable != 0) {