perf(packet): wire SIMD Internet checksum for long payloads - #492
perf(packet): wire SIMD Internet checksum for long payloads#492dyxushuai wants to merge 2 commits into
Conversation
Route checksum_add through the existing NEON/SSSE3 accumulator for buffers of 64 bytes or more; short headers stay scalar. Add a small microbench example for scalar vs SIMD throughput.
Mark the legacy NEON entry #[deprecated] and use consistent expression-style cfg branches in checksum_add_fast.
There was a problem hiding this comment.
Pull request overview
This PR speeds up ArcBox’s Internet-checksum hot path by wiring the existing NEON/SSSE3 implementations into checksum_add for long buffers, improving TCP/UDP full-segment checksum throughput while keeping short headers on the scalar loop to avoid SIMD setup overhead.
Changes:
- Add a length threshold and route
checksum_addto a new “fast path” that selects NEON on AArch64 and SSSE3 on x86_64 (with scalar fallback). - Refactor the SIMD routines to return the raw ones’-complement sum (
u32) and keep folding/complementing centralized inchecksum_fold. - Add a microbench example plus additional long-buffer correctness tests to guard the new dispatch behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| common/arcbox-packet/src/checksum.rs | Adds SIMD dispatch for long buffers, refactors SIMD helpers to return sums, and extends tests to cover long-payload correctness. |
| common/arcbox-packet/examples/checksum_bench.rs | Adds a small throughput microbench comparing scalar vs SIMD checksum computation. |
| /// Deprecated name kept for external callers that linked the old NEON entry. | ||
| #[cfg(target_arch = "aarch64")] | ||
| #[target_feature(enable = "neon")] | ||
| #[inline] |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two minor points inline.
Reviewed changes — wires the already-existing NEON/SSSE3 checksum kernels into the common checksum_add for payloads >= 64 bytes, so TCP/UDP full-segment checksums use SIMD on large buffers.
- SIMD dispatch in
checksum_add— buffers>= SIMD_THRESHOLD(64 B) route tochecksum_add_fast; shorter stay scalar. This is the hot path behindchecksum,tcp_checksum,udp_checksum. - Kernels return raw
u32sum —checksum_add_neon/checksum_add_ssse3now return the un-folded ones'-complement sum so partial sums compose with the pseudo-header before one finalchecksum_fold; switched toslice::as_chunks::<16>(). - API tidy —
checksum_simdkept as a pub test/bench entry,checksum_simd_neonretained as a pub-unsafe shim, and the old comment noise stripped. - Tests + bench — added long-buffer scalar/SIMD equivalence tests (4096 B and a 1500 B TCP segment) and a
checksum_benchexample.
Correctness checks out: scalar and SIMD both compute the true arithmetic word sum, and the u32 accumulators don't wrap below ~512 KB (NEON) / ~1 MB (SSSE3) — far above any packet — so the final fold is identical. The added tests pin this down.
Claude Opus ｜ 𝕏
| #[cfg(target_arch = "aarch64")] | ||
| { | ||
| // SAFETY: NEON is mandatory on AArch64. | ||
| return unsafe { checksum_add_neon(data) }; |
There was a problem hiding this comment.
ℹ️ The #[cfg(target_arch = "aarch64")] arm ends in a bare return that becomes the function's only statement after cfg-stripping, which can trip clippy::needless_return on the aarch64 target. Since that lint is -D warnings and aarch64 (Apple Silicon) is the P0 platform, an x86_64 CI run wouldn't catch it. Please confirm cargo clippy is clean on aarch64, or add #[allow(clippy::needless_return)].
| #[cfg(target_arch = "aarch64")] | ||
| #[target_feature(enable = "neon")] | ||
| #[inline] | ||
| pub unsafe fn checksum_simd_neon(data: &[u8]) -> u16 { |
There was a problem hiding this comment.
ℹ️ checksum_simd_neon has no in-repo callers, no #[deprecated] attribute, and the "external callers" rationale is thin for an alpha crate where CLAUDE.md states breaking changes are acceptable. Consider dropping it rather than carrying a dead pub-unsafe shim.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — this run covers commit 82b17fe, which addresses both minor points from the prior review.
checksum_add_fastaarch64 arm — the cfg-gatedreturnwas dropped in favor of a bare block tail expression, removing theclippy::needless_returnrisk on the P0 aarch64 target.checksum_simd_neon— kept for backward compat but now carries an explicit#[deprecated]attribute and a doc pointer tochecksum/checksum_simd, so the retained shim is intentional rather than stray dead code.
Both resolutions are clean and introduce no new behavior — the SIMD dispatch and correctness properties reviewed previously are unchanged.
Claude Opus ｜ 𝕏
| pub fn checksum_add(data: &[u8]) -> u32 { | ||
| if data.len() >= SIMD_THRESHOLD { | ||
| checksum_add_fast(data) | ||
| } else { | ||
| checksum_add_scalar(data) | ||
| } |
Greptile SummaryThis PR enables SIMD checksum accumulation for long packet buffers. The main changes are:
Confidence Score: 5/5This looks safe to merge. No blocking issues found in the changed code. Architecture gating and the scalar fallback cover supported targets. SIMD remainder and odd-byte handling match the scalar implementation.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (1): Last reviewed commit: "refactor(packet): deprecate checksum_sim..." | Re-trigger Greptile |
PeronGH
left a comment
PeronGH
left a comment
There was a problem hiding this comment.
Thanks for this — nice catch. checksum_add really was leaving the existing NEON/SSSE3 code unreachable, and the refactor to have the SIMD paths return the unfolded sum (rather than a folded u16) is the right shape: it composes cleanly with the pseudo-header accumulation in tcp_checksum/udp_checksum, which the old fold-inside-SIMD API couldn't. The example bench is a welcome addition too.
One thing worth measuring before this lands: our P0 target is macOS Apple Silicon, and the numbers look different there. On an M-series host I get (release, 3 runs, very reproducible):
| len | scalar | this PR (NEON) | |
|---|---|---|---|
| 64 B | 89 Gbit/s | 130 | 1.5x |
| 1500 B | 127 | 166 | 1.3x |
| 9000 B | 150 | 115 | 0.77x |
| 16 KiB | 171 | 112 | 0.66x |
| 64 KiB | 190 | 124 | 0.65x |
So it's a win at MTU size but a regression above ~4 KiB. That matters because the hottest consumer is finalize_virtio_net_checksum (virt/arcbox-vmm/src/device/checksum.rs), and we advertise VIRTIO_NET_F_CSUM unconditionally alongside GUEST_TSO4/6 — so guest TX hands us GSO segments up to 64 KiB, which is exactly the range that regresses.
The cause looks like the accumulator rather than the idea: checksum_add_neon chains every vpadalq_u16 onto the previous one, so the loop is latency-bound instead of throughput-bound, while LLVM already auto-vectorizes the scalar loop pretty well on aarch64 (scalar throughput climbs with length, 89 -> 190 Gbit/s, which is the tell). I tried a variant with several independent accumulators summed at the end and it went to ~750 Gbit/s at 64 KiB — 2.7x over scalar, byte-identical results — so there's a lot on the table here. The SSSE3 path has the same shape with 2 accumulators, so it may have headroom as well.
Smaller notes:
cargo clippy -p arcbox-packetwarnsmissing_safety_doconchecksum_simd_neon, and we gate on zero warnings. Sincergfinds no callers of it anywhere in the workspace and the crate is unpublished, I'd suggest just deleting it rather than deprecating — breaking changes are explicitly fine here (see CLAUDE.md).SIMD_THRESHOLD = 64with>=means a 64 B buffer does take the SIMD path, so the x86 0.9x at 64 B in your table applies rather than being avoided. Might be worth65, or measuring where the crossover actually sits.test_checksum_add_fast_matches_scalar_longuses 4096 B, an exact multiple of 16, so the SIMD remainder/odd-byte tail isn't covered (test_checksum_simdat 100 B leaves an even 4-byte remainder). I swept lengths 0..=600 against scalar locally and everything agrees — the code is correct — but a small length-sweep test would lock that in, since the tail is where checksum bugs like to hide.
Happy to help dig into any of this if useful.
checksum_addon long buffers still used the scalar loop even thoughNEON / SSSE3 implementations already existed. TCP/UDP full-segment
checksums (and anything else going through
checksum_add) pay thatcost on large payloads.
For buffers ≥ 64 bytes, accumulate with the existing SIMD path (return
the ones'-complement sum, then fold as before). Shorter headers stay
scalar so setup cost does not dominate. No new dependencies.
Microbench on x86_64 (release), scalar vs SIMD:
Correctness: long buffers match pure scalar sum; existing checksum
tests still pass.
This is orthogonal to inject IRQ coalescing — do not expect multi-flow
iperf to move from this alone.