Skip to content

Follow-ups from #813: the SEND_ZC hold's memory-bound comment after #805, a WARN that becomes a Debug line under the #801 lag #845

Description

@FumingPower3925

Follow-ups from the independent review of #813's rebase (50efff7 -> c417fe3 on main fe9264f; verdict: approve: 1 MINOR, 2 NITs). CodeRabbit's review of c417fe3 generated no actionable comments. Line references are at #813's head c417fe3. None of them blocks #813: the hold is correct, and each item below is either a comment, a body text or a narrow accounting case on the safe side.

  1. (MINOR) The comment's memory bound is out of date after fix(epoll, iouring): send a large response whole, keep pipelined responses in order, never send a body the handler has given back (celeris#761, celeris#802, celeris#817) #805. engine/iouring/zc_send_buffer.go:59-63 (and zcHoldBytesMax's comment at :125-133) says the held bytes "can then grow only by the SEND_ZCs already in flight on live connections, which the connection limit bounds". That bounds the number of sends, not the bytes. Since fix(epoll, iouring): send a large response whole, keep pipelined responses in order, never send a body the handler has given back (celeris#761, celeris#802, celeris#817) #805, sendCap returns math.MaxInt for HTTP/1 (conn.go:56-63) and every body is copied whole into writeBuf, so one SEND_ZC, and the one hold it becomes, can be a whole response of any size (an HTTP/2 connection's up to 64 MiB). Measured in the review: one 256 KiB response became one hold of 270,336 bytes. About 62 such stalled peers in flight at once, or a single response of 16 MiB or more, push a worker over the 16 MiB cap, and it then copies every send for as long as the orphaned socket lives. Not a correctness problem (the hold is required), and fix(iouring): hold a closed connection's send buffer until its SEND_ZC notification, past the release backstop (celeris#812) #813's Cost and Limits sections already say the cap does not bound sends already in flight. To do: reword the comment to "connections x their response size", and either count in-flight SEND_ZC bytes against zcHoldBytesMax or cap the size of a single SEND_ZC.

  2. (NIT) fix(iouring): hold a closed connection's send buffer until its SEND_ZC notification, past the release backstop (celeris#812) #813's body listed main's changed functions incompletely. It named completeSend, handleSend, closeConn and checkTimeouts, and left out releaseConnState (conn.go:545: main added three resets there; fix(iouring): hold a closed connection's send buffer until its SEND_ZC notification, past the release backstop (celeris#812) #813 calls it from releaseHeldConnState, zc_send_buffer.go:203, and edits its comment) and run(). The review checked it is harmless: fix(iouring): hold a closed connection's send buffer until its SEND_ZC notification, past the release backstop (celeris#812) #813 reads none of the reset fields. Fixed in fix(iouring): hold a closed connection's send buffer until its SEND_ZC notification, past the release backstop (celeris#812) #813's body before the merge; recorded here for the trail.

  3. (NIT) In a narrow case the backstop's WARN becomes a Debug line. The closing-drain teardown in checkTimeouts (worker.go:6146-6148) with zc_send_buffer.go:169. This is the fix(iouring): stop a ring SEND completion parking the worker on a running async handler's detachMu (celeris#750) #801 over-count fix(iouring): hold a closed connection's send buffer until its SEND_ZC notification, past the release backstop (celeris#812) #813's body describes: the closing-drain teardown in checkTimeouts does not replay held send completions, so suppose it finds a SEND_ZC notification held, and a recv is still owed past the 5 s backstop. Then ce.inflight == zcOwed matches wrongly, and the connState goes to the pool with the recv still owed, which is what main's backstop does too. The differences: a Debug line instead of the backstop's WARN (worker.go:4485), and CloseZCNotifHeld counts a hold for a notification that has already arrived. Safe side; needs a kernel anomaly. The fix belongs with fix(iouring): stop a ring SEND completion parking the worker on a running async handler's detachMu (celeris#750) #801's follow-ups (Follow-ups from #801: an end-to-end held-completion witness, the replay's place at every hand-back entry, two untested defensive paths, a 15 s wait without #800 #814): replay the held completions in the checkTimeouts teardown.

Outside #813, on main too (for #814): the same #801 lag makes fdOps (fd_lifetime.go:456) count a held SEND_ZC first CQE as still naming the descriptor. That connection then misses #798's early descriptor release, and past the backstop CloseFDForced (which must stay 0) counts once, on main and on #813 alike.

Also pending, and not a follow-up item: #813's cost with nothing held (one load and compare per send in prepSendSQE, a few branches per closed connection with an op owed) is queued as a bare-metal same-run ABA with an A/A twin (cluster row 63, reader analyze_cluster813.py), as a post-merge confirmation.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/engineEngine interface or implementationbugSomething isn't workingengine/iouringio_uring engine specifics

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions