perf(packet): wire SIMD Internet checksum for long payloads - #492
perf(packet): wire SIMD Internet checksum for long payloads#492dyxushuai wants to merge 3 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. |
![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 ｜ 𝕏
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 ｜ 𝕏
Greptile SummaryThe PR routes checksum accumulation for long buffers through architecture-specific SIMD implementations while preserving scalar fallbacks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains within the eligible follow-up-review scope.
|
| Filename | Overview |
|---|---|
| common/arcbox-packet/src/checksum.rs | Adds long-buffer SIMD dispatch, architecture-safe fallbacks, refactored accumulation routines, and expanded correctness tests. |
| common/arcbox-packet/examples/checksum_bench.rs | Adds a standalone microbenchmark comparing dispatched, scalar, and SIMD checksum throughput. |
Reviews (2): Last reviewed commit: "fix(packet): avoid checksum SIMD regress..." | 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
common/arcbox-packet/src/checksum.rs:18
- The
SIMD_THRESHOLDdocs read as if SIMD is used at 64 bytes, butchecksum_addonly routes to the fast path whenlen > SIMD_THRESHOLD(65+). Please adjust the wording so the boundary is unambiguous.
/// Length at which SIMD `checksum_add` is used. Short headers stay scalar
/// so setup cost does not dominate.
const SIMD_THRESHOLD: usize = 64;
There was a problem hiding this comment.
Important
The correctness work in d533def is solid, but the Apple Silicon regression this commit exists to fix is still unmeasured.
Reviewed changes — this run covers d533def, which responds to the assignee's benchmark review and to the >= threshold thread.
- Threshold flipped to strict
>— a 64 B buffer now stays on the scalar loop, and the bench sweeps 63/64/65 to pin the boundary. - NEON kernel re-shaped into four independent accumulators —
as_chunks::<16>()thenas_chunks::<4>(), withgroup_remainderfolded intosum0and avaddq_u32tree reduction, replacing the single serializedvpadalq_u16chain. - Big-endian AArch64 routed to scalar —
checksum_add_fastgained atarget_endian = "big"arm, sincevrev16q_u8assumes a little-endian host. - Deleted the
checksum_simd_neonshim — no in-repo callers, andchecksum_simdcollapsed to a single non-cfg'dfold(checksum_add_fast(..)). - Added a 0..=600 length sweep —
test_checksum_paths_match_scalarcheckschecksum_add,checksum_add_fast, andchecksum_simdagainst scalar at every length, which covers the group/group_remainder/odd-tail boundaries the old 4096 B test missed.
I worked the NEON arithmetic through independently and it is identical to scalar at every length: vrev16q_u8 + vreinterpretq_u16_u8 reproduces u16::from_be_bytes, the odd-byte tail matches, and as_chunks cannot mis-slice. Accumulator overflow is bounded by the final vaddvq_u32, which wraps only above ~128 KiB of input — unchanged from the single-accumulator version, and 2x above the 64 KiB maximum checksummed span. CI runs on macos-26, so the new length sweep does exercise the NEON path.
⚠️ The P0 regression this commit targets has no Apple Silicon measurement
The assignee measured the previous NEON path at 0.77x / 0.66x / 0.65x versus scalar at 9 KiB / 16 KiB / 64 KiB on an M-series host. That is precisely the size range that reaches finalize_virtio_net_checksum once GUEST_TSO4/6 is negotiated, and macOS Apple Silicon is the P0 platform. The four-accumulator rewrite is the right shape for that diagnosis, but the PR's Tests section reports only an x86_64 benchmark run plus an aarch64 clippy cross-check — nothing that would show whether the fix landed. If it under-delivers, this merges a ~35% regression on the hottest datapath in the tree.
Technical details
# Apple Silicon benchmark evidence is missing for a P0 perf change
## Affected sites
- `common/arcbox-packet/src/checksum.rs:211` — `checksum_add_neon`, the kernel the
four-accumulator rewrite targets. Only reachable on aarch64.
- `common/arcbox-packet/src/checksum.rs:27` — `checksum_add` dispatch; every caller
of `checksum` / `tcp_checksum` / `udp_checksum` routes through here above 64 B.
- Consumer chain: `virt/arcbox-vmm/src/device/checksum.rs::finalize_virtio_net_checksum`
-> `arcbox_net::nat_engine::checksum::{tcp_checksum, udp_checksum}`
-> `arcbox_packet::checksum::checksum_add`.
## Required outcome
- An M-series run of `cargo run -p arcbox-packet --example checksum_bench --release`
posted on the PR, showing the `checksum` / `scalar` / `simd` rows at 9000, 16384
and 65536 bytes.
- The dispatched (`checksum`) row must be at least at parity with `scalar` across
that whole range, not just at 1500 B. If it is not, the threshold needs an upper
bound, or the kernel needs more work, before this lands.
## Open questions for the human
- `checksum_add_ssse3` still carries two serialized `_mm_add_epi32` accumulators.
Is leaving x86_64 alone a deliberate deferral (P2 platform) or an oversight?ℹ️ Nitpicks
common/arcbox-packet/src/checksum.rs:16— see the inline comment; theSIMD_THRESHOLDdoc line is now off by one relative to the strict>dispatch.
Claude Opus ｜ 𝕏
| /// Length at which SIMD `checksum_add` is used. Short headers stay scalar | ||
| /// so setup cost does not dominate. | ||
| const SIMD_THRESHOLD: usize = 64; |
There was a problem hiding this comment.
Nit: with the dispatch now on strict >, 64 is the longest length that stays scalar, so "length at which SIMD is used" reads one off. The checksum_add doc above already says "longer than"; worth matching here.
| /// Length at which SIMD `checksum_add` is used. Short headers stay scalar | |
| /// so setup cost does not dominate. | |
| const SIMD_THRESHOLD: usize = 64; | |
| /// Longest buffer that stays on the scalar loop — `checksum_add` uses SIMD | |
| /// only above this length, so setup cost does not dominate short headers. | |
| const SIMD_THRESHOLD: usize = 64; |
Why
Long checksum buffers still paid the scalar accumulation cost. The original dispatch also sent 64-byte buffers through SIMD even though that path was slower on x86_64, and the AArch64 NEON accumulator serialized every chunk for long-buffer workloads.
How
checksum_simd_neoncompatibility shim.Tests
cargo test -p arcbox-packet --all-targetscargo clippy -p arcbox-packet --all-targets -- -D warningscargo clippy -p arcbox-packet --target aarch64-unknown-linux-gnu --all-targets -- -D warningscargo run -p arcbox-packet --example checksum_bench --releaseThe x86_64 benchmark keeps 64-byte inputs on the scalar path; 1500-byte and 64 KiB inputs remain about 2x faster than scalar in the local release run.