revert(iouring): restore the pre-#511 recv-pause branch - #518
Merged
Conversation
…rker #511 added a re-enqueue when getCancelSQE could not obtain an SQE, so the pause would be retried. That was wrong in two ways, and it regressed main. The worker loop calls drainDetachQueue on every iteration. Re-enqueueing the conn and re-arming detachQPending therefore makes the worker drain the queue again immediately instead of reaping the completions that would free the ring -- and because the append happens while the drain iterates its swapped-out spare slice, each pass appends the SAME conn again, so the queue grows without bound for as long as the ring stays full. The symptom is the celeris#482 regression guard failing on io_uring with close-timeouts: TestBackpressurePauseDoesNotCancelInflightSend reported conns that never completed their Close handshake, on kernel 6.17 CI runners where io_uring probes at tier=optional and the ring fills more readily than on the cluster. It failed on two PRs that touch neither io_uring nor WebSockets (#513 ratelimit, #515 h1), which is what identified #511 rather than either of them as the cause. Drop the re-enqueue and keep the correctness property: still do NOT record the conn as paused when the cancel was not submitted, because its multishot recv is still kernel-armed and a resume would arm a second one. The pause is simply deferred -- recvPauseDesired stays true and recvPaused stays false, so the next drain retries. A conn under backpressure is by definition still moving data, so another drain is imminent. Until then recv stays armed, which costs throughput on one conn but is always safe, unlike a phantom pause.
My #511 stopped marking a conn paused when the cancel SQE could not be obtained. With no retry that means the middleware's requested pause NEVER takes effect: recvPauseDesired stays true, recvPaused stays false, and PauseRecv only enqueues on the false->true transition, so nothing tries again. Under sustained backpressure the peer keeps filling a buffer nobody drains and the conn hangs -- which regressed the celeris#482 guard with close-timeouts on kernel 6.17 CI runners, where io_uring probes at tier=optional and the ring fills far more readily than on the cluster at tier=high. The first attempt at a fix re-enqueued the conn instead. That was worse: the worker loop drains the queue every iteration, so it spun on that conn rather than reaping the completions that would free the ring, and each pass appended the same conn again. Restore the shipped semantics -- mark the conn paused unconditionally -- and keep the one genuine improvement from #511: getCancelSQE submits and retries once, so the cancel actually lands far more often than with a bare GetSQE. The residual risk is the one #482 documented: on a full ring the recv is still kernel-armed while we record it as paused, so a later resume can arm a second multishot recv. That is pre-existing, rare, and strictly less harmful than a pause that never happens at all.
revive's empty-block fired on the sqe == nil arm, which after restoring the unconditional pause assignment held only the explanation. Invert to `if sqe := w.getCancelSQE(); sqe != nil` and keep the reasoning above the branch, where it explains the unconditional assignment rather than an empty arm. No behaviour change.
Diagnostic revert. #511 changed two things in drainDetachQueue's pause branch: the bare w.ring.GetSQE() became w.getCancelSQE() (which submits and retries), and the cs.recvPaused assignment became conditional. Restoring the conditional assignment alone did NOT fix the celeris#482 guard on CI, so this reverts the hunk to ed3d0af verbatim to establish whether #511 is the cause at all.
FumingPower3925
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #511, which regressed the celeris#482 backpressure guard on CI.
Result of the experiment
This branch restores
drainDetachQueue's pause branch toed3d0afverbatim. CI is green, 10/10. #511 was the cause.Which half
#511 changed two things:
w.ring.GetSQE()→w.getCancelSQE()— which callsw.ring.Submit()when the ring is fullcs.recvPaused = desiredbecame conditional on obtaining the SQE#516 restored (2) alone and still failed, so (1) is the culprit:
getCancelSQEsubmits from inside the drain loop, flushing a partially-built batch mid-iteration. The close path can afford that; this one cannot.What I got wrong, recorded so it is not repeated
The cluster passes this test at io_uring
tier=highunder every one of those variants. Only the constrained CI runner (kernel 6.17 azure,tier=optional) exposes it — so cluster verification alone is not sufficient for io_uring engine changes.The concern #511 tried to address
On a full ring the recv stays kernel-armed while the conn is recorded as paused, so a later resume can arm a second multishot recv. That risk is pre-existing and unchanged by this revert. It is worth fixing properly — but not with a mid-drain
Submit(). Filed separately.