Repository navigation
keccakf_amd64: drop NOSPLIT so the runtime can preemp assembly call - #35
Conversation
#32 removed NOSPLIT from the amd64 kernel so the runtime regains a preemption point in the absorb and squeeze loops. Nothing stops it coming back, and the failure is invisible in throughput benchmarks: it only shows up as a stop-the-world pause in whatever else the process is doing. TestKernelKeepsStackCheck reads the TEXT directives and rejects both ways of losing the stack-growth check: NOSPLIT, and a frame under 128 bytes, which makes the assembler mark a leaf NOSPLIT on its own. It covers both architectures from any runner, so an arm64 CI job still catches an amd64 regression. TestSTWPauseWhileHashing measures the property itself from the runtime's /sched/pauses/stopping/gc:seconds histogram. Its budget is relative: it hashes the same buffer through the pure-Go fallback first, which always has a preemption point, and only fails if the assembly path stalls the world four times longer than that and past 5ms. A slow or loaded runner moves both numbers together; only a real regression separates them. Verified by putting NOSPLIT back, assembly against the pure-Go floor: amd64 (EPYC 4344P) 20.48 us -> 83.886 ms arm64 (M4 Max) 327.68 us -> 100.663 ms, floor 163.84 us
…togram The stopping-pause histogram is cumulative for the life of the process, so picking its highest non-empty bucket attributes any earlier pause -- another test, the warm-up -- to the code under test. Taking bucket deltas fixes that but needs a sleep afterwards so a blocked stop-the-world is recorded before the snapshot, which is easy to get wrong. Timing runtime.GC() from another goroutine measures the same thing without a histogram, and the budget is already relative to the pure-Go fallback, so the extra precision was not buying anything. Verified both ways on both architectures. Healthy: 327us assembly against 668us pure Go on M4 Max, 416us against 526us on EPYC 4344P. With NOSPLIT put back: 91.9ms against 1.1ms, and 73.8ms against 596us. Matches the same test in erigon and klauspost/compress.
…t the bl…" This reverts commit 9d4ac7d.
AskAlexSharov
changed the title
AskAlexSharov
changed the title
There was a problem hiding this comment.
Pull request overview
Restores cooperative preemption for amd64 Keccak assembly calls and adds regression coverage.
Changes:
- Removes
NOSPLITfrom the amd64 kernel. - Updates the assembly generator accordingly.
- Adds static and runtime preemption regression tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
keccakf_amd64_bmi2.s |
Restores the stack-growth check. |
gen_keccakf_bmi2.go |
Keeps generated assembly consistent. |
keccak_asm_test.go |
Adds preemption regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Also fix the generator comment: the amd64 frame holds the round temp state, it is not unused.
textflag.h defines NOSPLIT as 4 and the assembler accepts the number, so `TEXT ·keccakF1600BMI2(SB), 4, $200-16` dropped the stack check while the guard stayed green - objdump shows zero morestack references.
FoundThe guard test passed while the guard was gone. builds a kernel whose prologue is FixedReject the whole flag field rather than one spelling — nothing legitimate belongs there for these kernels, and matching by name is what let the numeric form through. 5f0b3f8 Verified both ways round: Verified cleanThe assembly change itself is right: the dropped NOSPLIT produces the intended
|
Matching a name meant a renamed kernel matched nothing, and the test reported the rename rather than the missing check. Match any TEXT symbol: these files have no symbol that may keep NOSPLIT.
Found (follow-up)The guard keyed on a name, not on the property. The regex matched FixedMatch any Verified: renaming |
| // | ||
| // Either one lets a single large Sum256 or Write hold every P in | ||
| // stop-the-world for the length of the hash. | ||
| var kernelTEXT = regexp.MustCompile(`^TEXT\s+·(\w+)\(SB\),\s*(?:([A-Z0-9|+]+),\s*)?\$(\d+)-\d+`) |
There was a problem hiding this comment.
The stack-check guard silently skips any TEXT line its strict regex can't parse, as long as another TEXT line in the same file matches
TestKernelKeepsStackCheck only counts lines that fully match kernelTEXT. It fails a file only when that count is 0 (keccak_asm_test.go:93). The regex is stricter than the Go assembler and vet:
- It requires
$frame-argsize. The assembler accepts TEXT with no argument size (cmd/asm/internal/asm/asm.go: "The -argSize may be missing"). - Its flag class
[A-Z0-9|+]+rejects parentheses. \w+rejects an ABI selector such as·f<ABIInternal>(SB).^TEXTrejects an indented directive.
vet's asmTEXT regex (asmdecl.go:143) accepts all of the first three.
A TEXT line in any of these forms is never checked. If it is the only TEXT in its file, the found == 0 branch fires, with a misleading hint about #define. If the file also has a well-formed TEXT, the per-file counter is already non-zero, the unparsed line is skipped, and the test passes.
Concrete case: someone adds TEXT ·xorInBMI2(SB), NOSPLIT, $0 next to keccakF1600BMI2. That form is common in Go assembly. It assembles, and asmdecl does not complain: with NOSPLIT and size 0 it skips the arg-size check (asmdecl.go ~301). The guard stays green, yet a well-formed $0-16 spelling of the same line would fail it twice. Commit 2eb827b says the test now matches "any TEXT symbol: these files have no symbol that may keep NOSPLIT", and this gap breaks that promise. Neither .s file has such a line today, so this is a false negative in a regression guard, not a runtime defect.
Fix: Count every line matching a loose ^\s*TEXT\b. Report an error for any such line the strict regex rejects (e.g. "cannot parse TEXT directive") instead of skipping it. Then found == 0 is no longer the only way an unparsed line gets caught.
| // Do NOT add NOSPLIT here, and do not shrink the frame below 128 bytes. | ||
| // The frame holds the round temp state, and its stack check is the only |
There was a problem hiding this comment.
Generator comment makes 128 bytes the frame floor, but the kernel writes 200 bytes of scratch
The new comment says "do not shrink the frame below 128 bytes. The frame holds the round temp state". The test repeats it ("Keep the frame at 128 bytes or more", keccak_asm_test.go:90) and accepts any frame of 128 or more. But 128 is only the preemption floor. The real floor for this kernel is 200: off() (gen_keccakf_bmi2.go:175-180) emits idx*8(SP) for lanes 0-24, so the generated code writes up to 192(SP) whatever fsize is. The committed .s matches: its highest stack offset is 192(SP).
A maintainer who follows the comment and sets fsize = 128 gets a kernel that still passes TestKernelKeepsStackCheck. At run time the round code overwrites the saved BP at 128(SP), the return PC at 136(SP) and the caller's argument slots, and the program crashes at RET. The digest tests would crash right away, so the damage is contained, but the comment gives the wrong safe floor.
Fix: Say the frame must stay at 200 bytes (25 × 8 bytes of round scratch, which off() addresses up to 192(SP)). Mention abi.StackSmall (128) only as the separate reason a much smaller frame would also lose the stack check. Better, derive the offset limit from fsize so the two can't drift apart.
Re-lands #32, which was reverted by #34, and folds in the regression test from #33.
Problem
keccakF1600BMI2is declaredNOSPLIT, which drops the stack-growth check whatever the frame size. That check is the only cooperative preemption point in the absorb and squeeze loops that call it — the loops call nothing else, and assembly bodies are never async-preemptible (runtime/preempt.go#L450:isAsyncSafePointbails on anyFuncFlagAsmframe) — so one largeSum256orWriteholds every P in stop-the-world for roughly the length of the hash.Max GC stopping pause while hashing, EPYC 4344P,
GOMAXPROCS=2:Throughput is unchanged:
References: