drivers/usbdev/cdcncm: send TX immediately instead of after a tick-quantized delay#19469
drivers/usbdev/cdcncm: send TX immediately instead of after a tick-quantized delay#19469ricardgb wants to merge 1 commit into
Conversation
d18c44e to
d1e7f27
Compare
|
Please include the test logs. |
|
@ricardgb please fix the ci error: |
…d delay cdcncm_send() defers each transmit with MSEC2TICK(CDCNCM_DGRAM_COMBINE_PERIOD) (1 ms). MSEC2TICK() rounds up to the system tick, so at the default 100 Hz tick the "1 ms" coalescing window becomes a full 10 ms tick (10-20 ms with phase), adding that latency to every single-datagram reply (ICMP echo, TCP ACK, one-MSS HTTP segment) and dominating the CDC-NCM round-trip time. The window only coalesces datagrams appended within the same synchronous TX burst (already queued before the worker runs), so an inter-burst delay adds latency without batching benefit in the common case. Fire the transmit worker immediately (delay 0); within-burst coalescing is preserved. On RP2350 (Pico 2 W) USB-NIC at 100 Hz tick: ping RTT 21.7 -> 2.8 ms, a 257 KB HTTP download 5.79 -> 0.92 s (44.5 -> 279 KB/s). Signed-off-by: Ricard Rosson <ricard@groundbits.com> Assisted-by: Claude (Anthropic Claude Code)
d1e7f27 to
060ac41
Compare
|
@xiaoxiang781216 fixed — the |
|
@linguini1 on-hardware before/after from a Raspberry Pi Pico 2 W (RP2350), NuttX 13.0.0-RC0, USB CDC-NCM link (board
BEFORE — host → board, 20 pings (host µs clock): BEFORE — board → host, 10 pings (board 10 ms tick clock): AFTER — host → board, 30 pings: AFTER — board → host, 10 pings: Round-trip drops from 16.9 ms → 2.28 ms (host clock); the board-clock figure going from a flat 20 ms to sub-tick shows the tick-quantization the patch removes. Consistent with the 21.7 → 2.8 ms in the commit message. Disclosure: these tests were run and captured with the help of an AI agent (Claude Code, Anthropic); results are from real hardware and human-reviewed. |
| { | ||
| work_queue(ETHWORK, &self->delaywork, cdcncm_transmit_work, self, | ||
| MSEC2TICK(CDCNCM_DGRAM_COMBINE_PERIOD)); | ||
| /* Defer the transmit to the work thread so the caller is not blocked |
There was a problem hiding this comment.
another too long docstring. Do we really need so long docstring? Claude is very verbose by default
Summary
cdcncm_send()defers each transmit to the work queue with a coalescing delay ofMSEC2TICK(CDCNCM_DGRAM_COMBINE_PERIOD), whereCDCNCM_DGRAM_COMBINE_PERIODis1(ms).MSEC2TICK()quantizes to the system tick and rounds up, so on the default 100 Hztick (
CONFIG_USEC_PER_TICK=10000) a "1 ms" delay becomes a full 10 ms tick — 10–20 msof real dead time once tick phase is included. As a result the reply to every
single-datagram exchange (ICMP echo, TCP ACK, a one-MSS HTTP segment) is parked in the
work queue for ~10–20 ms before
EP_SUBMIT, which dominates the CDC-NCM round-trip time.Because the coalescing window only ever batches datagrams that the network stack appends
within the same synchronous TX burst (which are already queued before the worker runs),
a non-zero inter-burst delay adds latency without providing any batching benefit in the
common one-datagram-at-a-time case. This change fires the transmit worker immediately
(delay
0); within-burst coalescing is preserved.Impact
Measured on a Raspberry Pi Pico 2 W (RP2350) acting as a USB-NIC (CDC-NCM), default
100 Hz tick:
The RTT drop is the direct effect; the throughput gain follows because, without TCP write
buffers, the stack sends one segment per round trip, so throughput is
~MSS / RTT.Testing
Validated on hardware (RP2350 / Pico 2 W, CDC-NCM over USB FS to a Linux host): ping RTT
and HTTP download measured before/after as above; served content verified byte-identical
(sha256) after the change. Not yet exercised on other platforms; the reasoning is
tick-rate-general (any board at the default 100 Hz tick is affected).
Disclosure: this change was prepared with the assistance of an AI agent (Anthropic's
Claude, via Claude Code). The root cause, fix, and on-hardware measurements were
reviewed by a human maintainer before submission.