io_uring event engine — Stage 1: poll-mode readiness bridge - #30
io_uring event engine — Stage 1: poll-mode readiness bridge#30andypost wants to merge 8 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Linux io_uring event engine as an alternative to epoll, implementing a poll-mode readiness bridge using multishot poll capabilities. The review feedback identifies several critical and high-severity issues that need to be addressed: a potential buffer overflow and crash in nxt_io_uring_slot when handling negative file descriptors, a resource leak of the io_uring ring if initialization fails during the multishot support check, a potential compilation failure on systems with older liburing versions due to insufficient feature testing in the configure script, and an infinite loop risk in nxt_io_uring_pow2 for inputs exceeding 0x80000000.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| static nxt_io_uring_slot_t * | ||
| nxt_io_uring_slot(nxt_event_engine_t *engine, nxt_fd_t fd) | ||
| { | ||
| uint32_t nslots; | ||
| nxt_io_uring_slot_t *slots; | ||
| nxt_io_uring_engine_t *iou; | ||
|
|
||
| iou = &engine->u.io_uring; | ||
|
|
||
| if (nxt_slow_path((uint32_t) fd >= iou->nslots)) { |
There was a problem hiding this comment.
If fd is negative (e.g., -1), casting it to uint32_t results in 0xFFFFFFFF. This triggers the nxt_slow_path block where nxt_io_uring_pow2((uint32_t) fd + 1) evaluates to nxt_io_uring_pow2(0), returning 1. Consequently, nxt_realloc shrinks the slots array to a single slot, and the subsequent nxt_memzero is called with a huge negative size (1 - iou->nslots), causing a massive buffer overflow and an immediate crash. Adding a defensive check for negative file descriptors prevents this critical failure.
static nxt_io_uring_slot_t *
nxt_io_uring_slot(nxt_event_engine_t *engine, nxt_fd_t fd)
{
uint32_t nslots;
nxt_io_uring_slot_t *slots;
nxt_io_uring_engine_t *iou;
if (nxt_slow_path(fd < 0)) {
return NULL;
}
iou = &engine->u.io_uring;
if (nxt_slow_path((uint32_t) fd >= iou->nslots)) {There was a problem hiding this comment.
Fixed in b61f1b6: nxt_io_uring_slot() now rejects negative fds up front (all callers already tolerate NULL), so the uint32 cast can no longer shrink the table and memzero out of bounds.
|
|
||
| nxt_debug(&engine->task, "io_uring_queue_init(): sq:%uD cq:%uD features:%uxD", | ||
| iou->sq_entries, params.cq_entries, params.features); | ||
|
|
||
| if (!nxt_io_uring_multishot_supported(&iou->ring)) { | ||
| nxt_log(&engine->task, NXT_LOG_INFO, | ||
| "io_uring multishot poll is not supported"); | ||
| return NXT_ERROR; | ||
| } | ||
|
|
||
| iou->tier = NXT_IOU_TIER_POLL; |
There was a problem hiding this comment.
If nxt_io_uring_multishot_supported fails, nxt_io_uring_setup returns NXT_ERROR. At this point, iou->tier is still NXT_IOU_TIER_NONE. When nxt_io_uring_create handles the error and calls nxt_io_uring_free, the latter skips calling io_uring_queue_exit because iou->tier is not set to NXT_IOU_TIER_POLL. This leaks the initialized io_uring ring resources (file descriptors and kernel memory mappings). Setting iou->tier immediately after a successful io_uring_queue_init_params call ensures proper cleanup.
iou->tier = NXT_IOU_TIER_POLL;
nxt_debug(&engine->task, "io_uring_queue_init(): sq:%uD cq:%uD features:%uxD",
iou->sq_entries, params.cq_entries, params.features);
if (!nxt_io_uring_multishot_supported(&iou->ring)) {
nxt_log(&engine->task, NXT_LOG_INFO,
"io_uring multishot poll is not supported");
return NXT_ERROR;
}There was a problem hiding this comment.
Fixed in b61f1b6: the ring is now tracked with a dedicated ring_inited flag set immediately after a successful io_uring_queue_init_params(), and nxt_io_uring_free() keys io_uring_queue_exit() on it instead of the tier — the multishot-probe failure path no longer leaks the ring fd/mmaps. (Setting the tier early would have claimed POLL support before the probe proved it, so a separate flag is used.)
| nxt_feature_test="#include <liburing.h> | ||
|
|
||
| int main(void) { | ||
| struct io_uring ring; | ||
| int n; | ||
|
|
||
| n = io_uring_queue_init(8, &ring, 0); | ||
| if (n == 0) { | ||
| io_uring_queue_exit(&ring); | ||
| } | ||
| return 0; | ||
| }" |
There was a problem hiding this comment.
The configure test only checks for the presence of liburing and basic queue initialization. However, the engine relies on io_uring_prep_poll_multishot, which was introduced in liburing 2.2. If an older version of liburing is installed, the configure script will falsely report NXT_HAVE_IO_URING=YES, leading to compilation failures during the build phase. Adding a dummy call to io_uring_prep_poll_multishot in the configure test ensures the library is sufficiently new.
nxt_feature_test="#include <liburing.h>
int main(void) {
struct io_uring ring;
struct io_uring_sqe *sqe = NULL;
int n;
n = io_uring_queue_init(8, &ring, 0);
if (n == 0) {
io_uring_prep_poll_multishot(sqe, 0, 0);
io_uring_queue_exit(&ring);
}
return 0;
}"
There was a problem hiding this comment.
Fixed in b61f1b6: the configure test now compiles io_uring_prep_poll_multishot() and io_uring_sqe_set_data64() (the liburing >= 2.2 APIs Stage 1 uses), so an older liburing fails configure instead of the build. The Stage 2 PR extends the same test with io_uring_prep_cancel64().
| nxt_inline uint32_t | ||
| nxt_io_uring_pow2(uint32_t n) | ||
| { | ||
| uint32_t p; | ||
|
|
||
| p = 1; | ||
|
|
||
| while (p < n) { | ||
| p <<= 1; | ||
| } | ||
|
|
||
| return p; | ||
| } |
There was a problem hiding this comment.
The current loop-based implementation of nxt_io_uring_pow2 will enter an infinite loop if n > 0x80000000 because p will overflow to 0 and remain less than n forever. A safe, loop-free bit-manipulation approach is more efficient and completely avoids the risk of infinite loops.
nxt_inline uint32_t
nxt_io_uring_pow2(uint32_t n)
{
if (n <= 1) {
return 1;
}
n--;
n |= n >> 1;
n |= n >> 2;
n |= n >> 4;
n |= n >> 8;
n |= n >> 16;
n++;
return n;
}There was a problem hiding this comment.
Fixed in b61f1b6 with the branch-free round-up, plus an explicit saturation guard for n > 2^31 (unreachable from current callers, but now impossible to loop or overflow to 0 by construction).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a69098036
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /* Reject completions for a stale (disabled/closed) arming. */ | ||
| if (dir == NXT_IOU_DIR_READ) { | ||
| if (gen != slot->read_generation) { |
There was a problem hiding this comment.
When a fd slot is reused enough times, these comparisons reject every new completion: nxt_iou_ud() stores only 14 generation bits (NXT_IOU_GEN_MASK), but slot->read_generation/write_generation keep incrementing as 16-bit counters. After 16,384 disables/closes on the same fd number, a freshly armed poll encodes generation 0 while the slot still holds 16384, so all readiness CQEs for that reused low fd are dropped and connections can stall until the counter wraps much later. Compare the masked slot generation or keep the counters in the encoded range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b61f1b6: added nxt_iou_gen() and both dispatch-time comparisons now mask the slot counter, so a reused low fd keeps delivering completions past 2^14 arm/disarm cycles. (The counters intentionally stay free-running uint16_t; only the comparison is masked.)
|
|
||
| nxt_debug(task, "io_uring signalfd handler"); | ||
|
|
||
| n = read(ev->fd, &sfd, sizeof(struct signalfd_siginfo)); |
There was a problem hiding this comment.
When more than one signal is queued before this multishot poll is handled, reading only one signalfd_siginfo can strand the rest: unlike the existing epoll registration, this fd is armed through io_uring's edge-like multishot poll and the CQE path also deduplicates to one read handler per drain, so a still-readable signalfd is not guaranteed to be reported again. This can drop process-control signals such as a SIGTERM arriving while a SIGCHLD/SIGHUP is already pending; loop over read() until EAGAIN and dispatch each record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b61f1b6: the handler now drains the signalfd to EAGAIN, dispatching once per queued siginfo, and saves errno before nxt_debug() so logging cannot clobber the EAGAIN check. The signalfd was already O_NONBLOCK so the loop terminates.
| if (!nxt_io_uring_multishot_supported(&iou->ring)) { | ||
| nxt_log(&engine->task, NXT_LOG_INFO, | ||
| "io_uring multishot poll is not supported"); | ||
| return NXT_ERROR; |
There was a problem hiding this comment.
If io_uring_queue_init_params() succeeds but this create-time multishot probe fails, nxt_io_uring_create() calls nxt_io_uring_free(), but iou->tier is still NXT_IOU_TIER_NONE, so io_uring_queue_exit() is skipped and the ring fd/mmaps leak on the intended epoll-fallback path. This can happen under transient fd pressure, for example when the probe's eventfd() fails after the ring was created; mark the ring initialized before probing or explicitly exit it on this failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b61f1b6 (same fix as the ring-leak comment above): a ring_inited flag set right after io_uring_queue_init_params() succeeds; nxt_io_uring_free() exits the ring based on it, covering the probe-failure epoll-fallback path.
|
Review addressed in b61f1b6 ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b61f1b6914
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ev->read = NXT_EVENT_INACTIVE; | ||
| ev->write = NXT_EVENT_ONESHOT; | ||
|
|
||
| nxt_io_uring_arm(engine, ev, NXT_IOU_DIR_WRITE, 0); |
There was a problem hiding this comment.
When the sendfile path calls nxt_fd_event_oneshot_write() while a connection's normal read/write multishot polls are still armed, this only flips the read side to INACTIVE and submits another write poll. A read CQE can then be consumed while inactive; since slot->read_armed remains true, a later enable_read() will not submit a fresh POLL_ADD or queue the already-latched readiness, so pipelined data or FIN arriving during a sendfile write wait can be stranded and stall the keep-alive/static response flow. Remove/bump the existing pollers before arming the one-shot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2b5fbfe (branch rebased onto current master; new head d298a8c): oneshot_read/oneshot_write now cancel BOTH directions' live polls before arming the oneshot, reproducing epoll's EPOLL_CTL_MOD interest-mask replacement. The bug was slightly broader than described: arming a oneshot write while the write multishot was still live also created two concurrent write pollers on the same generation, so a residual multishot CQE could dispatch the write handler in DISABLED state. The remove bumps the generation (in-flight CQEs rejected), and a later enable_read() re-arms a fresh POLL_ADD that re-checks readiness at submission, so nothing is stranded.
| NULL, | ||
| NULL, | ||
| nxt_io_uring_enable_post, | ||
| nxt_io_uring_signal, |
There was a problem hiding this comment.
On builds where NXT_HAVE_SIGNALFD is false, this interface advertises no signal support so Unit starts the sigwait() signal thread, but nxt_event_engine_signal() still prefers the non-NULL .signal callback over the generic signal pipe. nxt_io_uring_signal() ignores nonzero signo values and only wakes the eventfd post path, so process-control signals delivered through the signal thread are dropped instead of dispatching their handlers; either leave .signal NULL without signalfd or handle nonzero signal numbers here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ab1080c: .enable_post and .signal are now installed together only under #if (NXT_HAVE_SIGNALFD), mirroring epoll's NXT_HAVE_EVENTFD tie (on Linux eventfd and signalfd always co-present). Without signalfd both are NULL, so posts AND the sigwait thread's signals route through the generic signal pipe and process-control signals are dispatched, not dropped.
| sqe = io_uring_get_sqe(&ring); | ||
| if (sqe != NULL) { | ||
| io_uring_prep_poll_multishot(sqe, 0, POLLIN); | ||
| io_uring_sqe_set_data64(sqe, 0); |
There was a problem hiding this comment.
This configure probe accepts liburing 2.2 because it only proves io_uring_prep_poll_multishot() and io_uring_sqe_set_data64() exist, but the new engine also calls io_uring_cq_has_overflow() in nxt_io_uring_poll(). That helper is absent from the liburing 2.2 public header, so ./configure --io-uring can succeed and the subsequent build fails on systems with exactly the documented minimum dependency; include this helper in the probe or raise the required version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d298a8c: the configure probe now also compiles io_uring_cq_has_overflow() — the newest liburing symbol the engine calls (added in liburing 2.3) — so ./configure --io-uring fails on liburing <= 2.2 instead of the subsequent build.
b61f1b6 to
d298a8c
Compare
|
Rebased onto current master (dddafac, post-1.36.0) — replayed cleanly, no conflicts; master's event-engine changes (active-connection tracking, graceful-quit plumbing) don't touch the fd-event interface this engine implements. The three pending review findings above are fixed and folded into the commits that introduced the respective code (new head d298a8c). Both configure variants build clean; the pytest subset passes identically on epoll and io_uring builds with the engine confirmed active in all processes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d298a8ca50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sa = &c->remote->u.sockaddr; | ||
| socklen = c->remote->socklen; | ||
|
|
||
| s = accept4(lev->socket.fd, sa, &socklen, SOCK_NONBLOCK); |
There was a problem hiding this comment.
When accept4() is available, every client accepted through the io_uring engine lacks FD_CLOEXEC because this path passes only SOCK_NONBLOCK. If an application process is spawned while the connection is open, it can inherit the client socket across execve(), retaining access to the connection and preventing normal teardown. The generic accept path explicitly sets FD_CLOEXEC, and the equivalent epoll path passes SOCK_NONBLOCK | SOCK_CLOEXEC; this new accept path must preserve the same invariant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — a real drift from the rebase: the shim predates master's CLOEXEC hardening of the accept paths. Fixed in 8fed0a0 (new head 77ed9c2): accept4() now passes SOCK_NONBLOCK | SOCK_CLOEXEC, mirroring nxt_epoll_conn_io_accept4() and the fcntl(FD_CLOEXEC) contract in nxt_conn_io_accept(). The nxt_io_uring_test_accept4 probe was checked for the same drift — it matches master's epoll probe (ENOSYS-only test, no flags needed).
| slot = nxt_io_uring_slot(engine, ev->fd); | ||
| if (nxt_slow_path(slot == NULL)) { | ||
| nxt_alert(ev->task, "io_uring slot alloc failed for fd %d", ev->fd); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When growing the fd-indexed slot table fails under memory pressure, this function returns without arming a poll, but its callers still leave the nxt_fd_event_t direction in NXT_EVENT_ACTIVE. The same silent outcome occurs if no SQE is available after submission. Because no CQE can then arrive to drive another enable attempt, an affected connection can stall indefinitely, and a listener passed through enable_accept() can stop accepting altogether; return an error or enqueue the event's error handler instead of representing the unarmed direction as active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 8fed0a0: nxt_io_uring_arm() now returns success, and every arm site (enable, enable_read/write, oneshot_*, enable_accept, and the CQE-handler re-arms) escalates failure through a new nxt_io_uring_arm_failed() → nxt_io_uring_error_handler queued on fast_work_queue — a faithful copy of nxt_epoll_commit_changes() → nxt_epoll_error_handler (both directions INACTIVE, then ev->error_handler), with a NULL guard for internal fds like the signalfd whose events have no error_handler. No direction can be left ACTIVE with no poller armed.
| ev->epoll_error = 1; | ||
| } | ||
|
|
||
| if (mask & NXT_IOU_READ_MASK) { |
There was a problem hiding this comment.
On Linux socket failures where poll reports only POLLERR/POLLHUP, this condition treats the error bits as read readiness because they are included in NXT_IOU_READ_MASK; the independently armed write poll does the same through NXT_IOU_WRITE_MASK. Both direction handlers can therefore be queued for one failed connection despite the per-direction dedupe, whereas the existing poll and epoll engines deliberately dispatch only one handler for an error-only result. If the first callback closes or frees the connection, the second queued callback can operate on stale state; distinguish POLLIN/POLLOUT readiness from error-only results and dispatch one error/active-direction handler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in c80fe4d: dispatch now keys on real readiness bits only (POLLIN / POLLOUT; a combined data+error CQE still goes to the readiness handler where recv()/send() surfaces the error, matching epoll clearing its error flag once EPOLLIN dispatched). ERR/HUP-only results route through a new nxt_io_uring_error() implementing nxt_epoll_poll()'s exact policy: suppressed unless a direction is still active (epoll's <= NXT_EVENT_BLOCKED test), and deduped per fd per CQ drain across the read poller, write poller, AND any readiness handler already dispatched — so one dead socket queues exactly one error_handler, never a second callback on state the first may have freed. The res<0 poll-op-failure path was rerouted through the same helper (it previously deduped only per direction, so it had the same latent double-queue). Policy documented in the code.
d298a8c to
77ed9c2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77ed9c2380
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nxt_io_uring_enable_read(engine, &iou->signalfd); | ||
|
|
||
| return NXT_OK; |
There was a problem hiding this comment.
When the signalfd's fd requires slot-table growth under memory pressure, or no SQE remains available, nxt_io_uring_enable_read() only queues nxt_io_uring_arm_failed() and this function still returns NXT_OK. The new escalation path is ineffective for this internal event because iou->signalfd.error_handler is NULL, so engine creation succeeds with no signal poll and process-control signals remain blocked without being dispatched. Arm the signalfd through a fallible path and propagate failure so creation can fall back to epoll.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 31ed402 (new head 962b72d): nxt_io_uring_add_signal() now arms the signalfd through the fallible nxt_io_uring_arm() directly and returns NXT_ERROR on failure, so create() unwinds (nxt_io_uring_free closes the signalfd and exits the ring; the sigmask stays blocked exactly as on epoll's own signalfd failure paths, and epoll's add_signal re-blocks and re-arms after fallback). Verified at runtime with an injected arm failure: alert → io_uring free → 'using "epoll_edge" event engine'.
| ev->read = NXT_EVENT_INACTIVE; | ||
| ev->write = NXT_EVENT_INACTIVE; |
There was a problem hiding this comment.
If nxt_io_uring_enable() successfully arms the read direction but the subsequent write arm fails, the added arm-failure callback reaches this handler while slot->read_armed is still true. Clearing both event states before invoking the owner's error callback means a later nxt_conn_close() calls nxt_io_uring_delete(), which skips both removes because both states are already inactive; the surviving multishot request can retain the socket and its dangling slot->ev pointer after the connection is freed. Cancel every armed slot direction before clearing the states, or make deletion consult the slot flags rather than only the event states.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 0dd481b: new bounds-checked nxt_io_uring_slot_armed() is OR'd into the delete and disable gates, so an fd whose ev states were force-cleared by the arm-failure escalation still gets its live pollers POLL_REMOVE'd (with the generation bump) before close/free. This is the systemic fix — delete/disable are the universal exit every fd traverses before nxt_socket_close, and unlike epoll (kernel auto-deregisters a closed fd) an io_uring poll holds a file reference until removed, with slot->ev pointing at the event. Audited the other inactivating paths: disable_read/write and oneshot_* already remove unconditionally; error_handler→delete window is safe (stray CQEs are dropped by the ev-state dispatch gates while the ev is still alive).
| force = getenv("NXT_IO_URING_FORCE_TIER"); | ||
|
|
||
| if (force != NULL | ||
| && (nxt_strcmp(force, "none") == 0 | ||
| || nxt_strcmp(force, "create") == 0)) |
There was a problem hiding this comment.
In a --io-uring-default build, setting the documented runtime kill switch NXT_IO_URING=0 has no effect because both the registration probe and create path inspect only NXT_IO_URING_FORCE_TIER. The process therefore still issues io_uring syscalls and selects this engine, defeating the advertised emergency mitigation for a kernel or seccomp regression; check NXT_IO_URING before probing or creating the ring and retain the force-tier variable only as the debug override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partially confirmed: NXT_IO_URING=0 wasn't actually documented as a runtime switch anywhere in the branch — the only similar text is the configure-time NXT_IO_URING=NO shell variable in auto/options (build option, not runtime). That said, an ops kill switch is cheap insurance for exactly the kernel/seccomp regression scenario you describe, so it's implemented in b36284f (registration probe → engine never registered) and 1afaf1f (create() → clean degrade with an INFO log, covering an explicitly-selected engine), and documented in the engine's header comment under 'Runtime control' next to the debug-only FORCE_TIER. Verified: NXT_IO_URING=0 on an --io-uring-default build starts with 'using "epoll_edge" event engine' and issues zero io_uring syscalls.
77ed9c2 to
962b72d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962b72d7c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sqe = nxt_io_uring_get_sqe(engine); | ||
|
|
||
| if (nxt_fast_path(sqe != NULL)) { |
There was a problem hiding this comment.
When the eventfd multishot poll terminates and nxt_io_uring_get_sqe() remains NULL after attempting to flush the SQ, this path silently drains the eventfd without recording that its poll is now unarmed. Subsequent cross-thread posts can write successfully but never wake an otherwise sleeping event loop, leaving locked-queue work stalled indefinitely. Fresh evidence beyond the earlier generic arm-site fix is that this separate internal re-arm path still ignores the NULL case; retain a retry or escalate the failure instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 574bdc3 (new head b261dd3): the failed doorbell re-arm now sets post_rearm_pending; nxt_io_uring_poll() retries it at the top of every iteration and, while it's owed, caps the wait at 100ms so a cross-thread post that wrote the eventfd against the dead poll is picked up within one cap interval (the retried multishot POLL_ADD re-checks the still-signaled eventfd at submission and fires immediately; the payload rides locked_work_queue, drained on every wake). Demonstrated with fault injection: doorbell dead → wait capped → re-armed via the poll-loop retry, posted config changes kept applying. One honest residual noted in this round: the signalfd's own (kernel-dropped multishot AND SQ-exhausted) double failure would leave it dead — tracked for a follow-up, out of this fix's scope.
| sqe = nxt_io_uring_get_sqe(engine); | ||
| if (nxt_slow_path(sqe == NULL)) { | ||
| nxt_alert(ev->task, "io_uring_get_sqe() failed for fd %d", ev->fd); | ||
| return; |
There was a problem hiding this comment.
When nxt_io_uring_get_sqe() remains NULL during a disable or close, this returns after the slot was already marked unarmed and its generation bumped. The live kernel poll is therefore never canceled and cannot be retried by later deletion; re-enabling can install an additional poll, while closing leaves the old request holding a reference to the socket until the entire ring is destroyed. Preserve the armed state for a retry or escalate the cancellation failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 433b077: the cancel is never lost, but we deliberately KEPT the mutate-first order rather than retaining armed state — a retained armed=1 after a failed remove would leave a live poll feeding gen-valid CQEs into a direction the caller believes disabled (the latched-readiness hazard class), and close() would hand a still-armed fd to close(2). Instead the pre-bump generation is recorded per direction (*_remove_pending/*_remove_gen) and retried from nxt_io_uring_poll(); nxt_io_uring_arm() flushes a pending cancel before installing a new poll (failing the arm and escalating if it can't), which provably bounds the state to one condemned arming per direction. Deferring the close(2) itself was evaluated and rejected: the only caller honoring the close return re-closes from a zero-timer without re-entering the engine, and it's unnecessary anyway — POLL_REMOVE matches by user_data, not fd, the kernel poll pins its own file reference, and an fd-number reuse arms under a bumped generation, so the deferred cancel lands correctly even after close(2). Demonstrated with fault injection (recorded → retried → submitted, requests unaffected).
962b72d to
b261dd3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b261dd307c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else | ||
| NXT_HAVE_IO_URING=NO | ||
| fi |
There was a problem hiding this comment.
When --io-uring or --io-uring-default is explicitly requested but the liburing probe fails, this branch silently disables the feature and allows configuration to succeed. I verified that ./configure --io-uring --no-regex exits 0 without liburing and reports io_uring support: NO, so packaging or CI can publish an epoll-only binary despite requesting the new engine; fail configuration here as other explicitly enabled optional dependencies do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed (reproduced: ./configure --io-uring exited 0 with "io_uring support: NO") and fixed in 776fbaa: the probe-fail branch in auto/events now errors out (exit 1) with a clear "no liburing library found" message when io_uring was explicitly requested, mirroring the auto/ssltls / auto/compression precedent. Verified with a CC wrapper that fails on -luring: explicit --io-uring now exits 1; a plain ./configure is unaffected.
| if (nxt_slow_path(!nxt_io_uring_arm(engine, ev, NXT_IOU_DIR_READ, | ||
| slot->read_multishot))) | ||
| { | ||
| nxt_io_uring_error(engine, slot, ev); |
There was a problem hiding this comment.
When a terminated listener POLL_ADD reports readiness but its replacement cannot obtain an SQE, slot->read_seq has already been set for this drain, so this call immediately returns through the dedupe in nxt_io_uring_error() without queuing the listener's error handler. The accept handler does not normally call enable_accept() again, leaving the listener ACTIVE with read_armed == 0 and silently stopping future accepts. Fresh evidence beyond the earlier generic arm-site fix is that the current same-drain dedupe specifically suppresses its newly added re-arm-failure escalation after readiness was queued; this failure must still be reported or retried.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a50e1ce: a transient SQ-exhaustion on a listeners re-arm no longer dies silently (the old escalation self-deduped against read_seq already set this drain). New pending-arm machinery (read/write_arm_pending + npending_arms + nxt_io_uring_arm_pend/retry_pending_arms) records the owed re-arm and retries it at the top of nxt_io_uring_poll(), before the wait, so it normally lands the same iteration. Invalidation: nxt_io_uring_remove() clears the pending record up front on any disable/delete/close/oneshot; the retry re-checks ev present + direction still wanted + not already armed and arms under the current generation. No healthy listener is killed for a transient exhaustion, and none is stranded ACTIVE-but-unarmed.
| ev->error = -res; | ||
|
|
||
| nxt_io_uring_error(engine, slot, ev); |
There was a problem hiding this comment.
When the live signalfd poll completes with an unexpected negative result, such as a runtime resource failure other than the excluded cancellation errors, this routes it through the generic error path. The internal iou->signalfd has no error_handler, so nxt_io_uring_error_handler() only marks it inactive; because the failed multishot request has terminated and nothing re-arms it, the process keeps its control signals blocked but can no longer deliver subsequent SIGTERM, SIGQUIT, or SIGCHLD events. Re-arm this internal poll or treat its failure as a fatal engine condition instead of applying the no-op owner callback path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a50e1ce: a current-generation negative res on the signalfd poll is now detected (ev == &iou->signalfd) and self-healed — the multishot read poll is re-armed (deferred via the pending-arm path if no SQE, with an alert for visibility) instead of routing to the NULL error_handler that would leave signals blocked-but-undelivered. Audited the eventfd doorbell (NXT_IOU_UD_POST) as well: it already self-heals via internal_cqe re-arm + post_rearm_pending, so no change there.
| if (ev->write != NXT_EVENT_INACTIVE | ||
| && slot->write_seq != engine->u.io_uring.drain_seq) |
There was a problem hiding this comment.
When one direction's error-only CQE is processed before the other direction's readiness-plus-error CQE, nxt_io_uring_error() records error_seq and queues the fast error handler, but this readiness gate does not consult error_seq and queues the write handler as well. This occurs, for example, when the read poll reports only POLLERR|POLLHUP while the write poll reports POLLOUT|POLLERR|POLLHUP; the error callback can close or otherwise invalidate the connection before the later write callback runs. Fresh evidence beyond the earlier error-only fix is that deduplication is asymmetric in the current code: errors are suppressed after readiness, but readiness is not suppressed after an error. Include error_seq in both readiness dispatch gates or merge the fd's CQEs before selecting handlers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed (folded into the review-findings commit, new head a50e1ce): both readiness dispatch gates now also check slot->error_seq != drain_seq, so once a directions ERR-only CQE has queued the fd error_handler this drain, a following readiness+error CQE on the other direction no longer queues a second handler. One handler per fd per CQ drain, matching epolls single-event semantics; the error handler (which sets both dirs INACTIVE) runs later from the work queue, so dispatch-time gating is the actual protection.
|
@claude please do a thorough code review of this PR. It adds a poll-mode io_uring event engine bridging Unit's readiness-based nxt_event_interface_t contract onto multishot IORING_OP_POLL_ADD. Focus on: (1) fidelity to the epoll engine's observable contract (edge-like delivery, oneshot semantics, error-only dispatch policy, BLOCKED handling); (2) fd/slot lifecycle — generation checks vs fd reuse, close vs in-flight CQEs, the deferred-cancel retry under SQ exhaustion; (3) failure paths — arm failures, signalfd/create unwind, epoll fallback, the NXT_IO_URING=0 kill switch; (4) the configure probe vs the liburing APIs actually used. Flag anything that could stall a connection, leak an fd/kernel reference, or double-dispatch a handler. CI is green and four rounds of bot review are already folded in — look for what those missed. |
|
@codex review — full review of the current head please, beyond the incremental push reviews: the poll-mode io_uring readiness bridge as a whole (epoll-contract fidelity, fd/generation lifecycle across close/reuse, SQ-exhaustion recovery paths, error dispatch policy, configure probe). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b261dd307c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (multishot) { | ||
| io_uring_prep_poll_multishot(sqe, ev->fd, mask); |
There was a problem hiding this comment.
A multishot request retains both this ev pointer and a kernel reference to the underlying file, but established port teardown bypasses nxt_fd_event_close(): nxt_process_close_ports() calls nxt_port_close(), which closes pair[0] directly (src/nxt_process.c:1235-1246, src/nxt_port.c:88-104). Consequently process-port teardown neither bumps the generation nor submits POLL_REMOVE; the endpoint remains open, and a later CQE can dispatch through a released nxt_port_t or a reused descriptor can inherit the still-armed slot and never receive its own poll. These direct-close paths must disarm the io_uring request before closing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — traced it: nxt_port_read_close (nxt_port_socket.c) and nxt_port_close (nxt_port.c) raw-close pair[0] without nxt_fd_event_close/delete, so an armed io_uring poll survives with the generation un-bumped and slot->ev dangling (harmless on epoll, which the kernel auto-deregisters). The honest fix — calling the engine from port teardown — is unsafe here because ports are closed cross-thread from port->engine (nxt_router_app_use posts free_app_work to app->engine), so it would submit to another threads ring. Fixed in a50e1ce with an engine-local self-heal: nxt_io_uring_slot_reconcile(), called from every arm entry point, detects slot->ev != ev on a still-armed reused fd, condemns both directions via nxt_io_uring_remove (generation bump + POLL_REMOVE) and arms fresh. This deterministically closes the fd-reuse stall and rejects every stale poll CQE once the fd is re-registered. Documented residual: a CQE landing in the narrow window BEFORE re-registration can still dispatch through the stale slot->ev; fully closing that needs a thread-safe port-teardown deregistration posted to port->engine — recommended follow-up, out of scope for an engine-local change.
| if (nxt_slow_path(!nxt_io_uring_arm(engine, ev, NXT_IOU_DIR_READ, | ||
| slot->read_multishot))) | ||
| { | ||
| nxt_io_uring_error(engine, slot, ev); |
There was a problem hiding this comment.
When a terminated poll delivers readiness and the replacement arm cannot obtain an SQE, the handler was already recorded in read_seq; therefore this call to nxt_io_uring_error() immediately returns at lines 1772-1775. For the non-multishot listener path this leaves ev->read ACTIVE while slot->read_armed is false, and nxt_conn_listen_handler() only starts the accept batch—it does not re-enable the event—so all subsequent connections remain unaccepted after the SQ-exhaustion event. Re-arm failure needs a retry record or an error path that is not suppressed by the readiness dedupe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a50e1ce: a transient SQ-exhaustion on a listeners re-arm no longer dies silently (the old escalation self-deduped against read_seq already set this drain). New pending-arm machinery (read/write_arm_pending + npending_arms + nxt_io_uring_arm_pend/retry_pending_arms) records the owed re-arm and retries it at the top of nxt_io_uring_poll(), before the wait, so it normally lands the same iteration. Invalidation: nxt_io_uring_remove() clears the pending record up front on any disable/delete/close/oneshot; the retry re-checks ev present + direction still wanted + not already armed and arms under the current generation. No healthy listener is killed for a transient exhaustion, and none is stranded ACTIVE-but-unarmed.
| if (ev->read != NXT_EVENT_INACTIVE | ||
| && slot->read_seq != engine->u.io_uring.drain_seq) | ||
| { |
There was a problem hiding this comment.
The two directional pollers can complete in either order during the same drain. If an ERR/HUP-only CQE is processed first, nxt_io_uring_error() queues the fast error handler and records error_seq; a later POLLIN CQE still passes this condition because it checks only read_seq, so both handlers are queued. The fast error handler runs first and may close or free the connection before the read handler executes, violating the single-dispatch policy and potentially causing a use-after-free. Read and write dispatch must also honor an error already selected for the current drain, or error selection must be deferred until both CQEs are consolidated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed (folded into the review-findings commit, new head a50e1ce): both readiness dispatch gates now also check slot->error_seq != drain_seq, so once a directions ERR-only CQE has queued the fd error_handler this drain, a following readiness+error CQE on the other direction no longer queues a second handler. One handler per fd per CQ drain, matching epolls single-event semantics; the error handler (which sets both dirs INACTIVE) runs later from the work queue, so dispatch-time gating is the actual protection.
| if (res < 0) { | ||
| /* -ECANCELED/-EBADF/-ENOENT after a remove or close: fd is gone. */ | ||
| if (res == -ECANCELED || res == -EBADF || res == -ENOENT) { | ||
| return; |
There was a problem hiding this comment.
A completion from a successful remove or engine-mediated close carries the pre-bump generation and is rejected above, so a current-generation -EBADF, -ENOENT, or -ECANCELED is not safely identifiable as stale. In particular, POLL_ADD can complete with -EBADF if the descriptor becomes invalid before the batched SQE is consumed; this branch has already cleared *_armed but leaves the event ACTIVE and silently returns, permanently stranding it. Route current-generation failures through the existing error/recovery path rather than treating them as removed requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a50e1ce: current-generation -ECANCELED/-EBADF/-ENOENT are no longer early-returned as stale. Reasoning verified and documented in-code: every POLL_REMOVE this engine issues (immediate and the deferred round-4 retry) targets the pre-bump generation, so a self-inflicted cancel is always dropped by the generation gate above; anything reaching res<0 with a matching generation is a genuine failure and now routes through nxt_io_uring_error() (after the signalfd self-heal check) rather than stranding an ACTIVE event.
|
|
||
| #define NXT_IOU_KIND_FD 0 | ||
| #define NXT_IOU_KIND_INT 1 | ||
|
|
There was a problem hiding this comment.
Only 14 generation bits are encoded in user_data, so under sustained CQ overflow an old readiness CQE can remain in the kernel overflow backlog while a low-numbered fd is closed, reused, and re-armed repeatedly; after 16,384 generation bumps, the stale CQE again matches the slot and is dispatched through the new owner's slot->ev. The engine explicitly allows re-arming before cancellation completion, and NODROP preserves rather than discards the backlog, so high connection churn during overload can cross this wrap boundary. Allocate enough generation bits to make aliasing impossible for outstanding requests, or defer slot reuse until the old arming's terminal completion is observed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a50e1ce: the generation field is widened from 14 to 32 bits. New user_data packing is dir(1) | kind(1) | gen(32, bits 2-33) | idx(30, bits 34-63) — non-overlapping, summing to 64, idx 30 bits covers any RLIMIT_NOFILE. All generation fields (read/write_generation, read/write_remove_gen) and submit_removes param are uint32_t, and nxt_iou_gen is now an identity so the compare is exact across the full 2^32 period, making stale-CQE aliasing unreachable for any realistic churn.
| if (ret < 0 && ret != -ETIME && ret != -EINTR && ret != -EAGAIN) { | ||
| nxt_alert(&engine->task, "io_uring_submit_and_wait_timeout() failed %E", | ||
| -ret); | ||
| return; |
There was a problem hiding this comment.
io_uring_enter can surface CQ/NODROP pressure as -EBUSY while completions are already available and need to be consumed before submission can progress. This branch treats -EBUSY as fatal and returns before the CQ traversal and io_uring_cq_advance(), so the full CQ remains unchanged and the next event-loop iteration can receive the same error indefinitely, stalling all I/O while repeatedly logging. Handle this recoverable result like -EAGAIN and always drain visible CQEs before returning from the poll iteration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a50e1ce: -EBUSY is now in the recoverable set alongside -ETIME/-EINTR/-EAGAIN. With IORING_FEAT_NODROP the kernel surfaces CQ-full backpressure as -EBUSY while completions are pending, so the poll iteration now drains the visible CQEs (freeing CQ space) and lets the next submit_and_wait re-issue the queued SQEs, instead of early-returning and livelocking.
Wire an opt-in io_uring event engine into the build system, mirroring the existing epoll/openssl plumbing: - auto/events: liburing compile+link probe gated on --io-uring, defining NXT_HAVE_IO_URING and, for --io-uring-default, NXT_IO_URING_DEFAULT. - auto/options: --io-uring / --no-io-uring / --io-uring-default flags. - auto/sources: build src/nxt_io_uring_engine.c when NXT_HAVE_IO_URING=YES. - configure: append -luring to the link line via NXT_IO_URING_LIBS. - auto/help, auto/summary: document and report the feature. Default stays OFF so existing builds are unchanged. Motivation is syscall-overhead reduction for C10K-class scalability.
Implement nxt_io_uring_engine as a drop-in nxt_event_interface_t that uses a
multishot IORING_OP_POLL_ADD per armed direction to feed Unit's existing
read_ready/write_ready latches and per-direction work queues. All
recv/send/accept syscalls stay in userspace, so the engine reuses the generic
level-emulating nxt_unix_conn_io with no edge-mode shims.
Mapping to the vtable, mirroring the epoll engine:
- enable/enable_read/enable_write/enable_accept: arm multishot POLL_ADD; a
re-enable from BLOCKED is a pure state write (zero SQEs) since the multishot
stays armed in the kernel.
- block_read/block_write: no SQE, state flag only; the poll loop latches
readiness but does not dispatch a BLOCKED direction, and disarms it
level-style if it keeps firing (avoids a busy-loop on a writable socket).
- disable_*/delete/close: POLL_REMOVE and a per-direction generation bump so
any stale completion (including after fd close/reuse) is dropped in the CQE
handler before the event object is touched. close() submits the removes
immediately.
- oneshot_*: single non-multishot POLL_ADD, ONESHOT -> DISABLED on completion.
- enable_post/signal: an eventfd armed with a multishot poll is the ring
doorbell; the cross-thread wake is the eventfd write(2), never a ring submit.
- poll: one io_uring_submit_and_wait_timeout flushes the batched SQEs and waits,
the thread clock is refreshed immediately, the CQ is drained with
io_uring_for_each_cqe + a single cq_advance, and CQ overflow (NODROP) forces a
non-blocking next loop.
user_data encodes {slot index == fd, per-direction 14-bit generation,
direction}; the side table is indexed by fd and grown on demand. create()
maps EPERM/ENOSYS/EMFILE/ENOMEM and a failed multishot-poll functional test to
a clean NXT_ERROR (no process-fatal path), and honors
NXT_IO_URING_FORCE_TIER=none.
The engine is registered by name and appended in nxt_services_init(), so epoll
remains the default. nxt_event_engine_create() now logs the selected engine at
info level (using "<name>" event engine) for both the main process and router
workers.
Add nxt_io_uring_probe(): a throwaway ring plus multishot-poll functional test that maps every "unsupported" condition (EPERM from seccomp or kernel.io_uring_disabled, ENOSYS, EMFILE/ENFILE, ENOMEM) and a missing IORING_CQE_F_MORE to NXT_ERROR, and honors NXT_IO_URING_FORCE_TIER=none. nxt_services_init() now registers the io_uring engine only when the probe passes, so an unavailable io_uring never exposes a broken engine name. A short retry in nxt_runtime_event_engines() closes the probe-vs-create TOCTOU window: if the io_uring engine is the default but its create() fails, the runtime falls back to the epoll engine instead of aborting startup.
Use the NXT_IO_URING_DEFAULT build define (emitted by --io-uring-default) to prepend the io_uring engine ahead of epoll in nxt_services_init() so the NULL-name lookup selects it as the default for the main process and every router worker. Without the flag the engine is appended and epoll stays the default. Both selections remain gated on nxt_io_uring_probe(), so a build with io_uring as default still falls back to epoll on a kernel that cannot run it. This yields two build flavours -- epoll-default (--io-uring) and io_uring-default (--io-uring-default) -- for clean A/B benchmarking without code edits.
The Stage-1 engine advertised no signal support while providing a native eventfd post channel. For a process created with signals (the main process) nxt_event_engine_create() therefore spawned the sigwait() signal thread, which routes real signals through nxt_event_engine_signal(engine, signo) -> engine->event.signal(). The ring's eventfd doorbell only wakes the loop and cannot carry the signo, so SIGTERM/SIGQUIT were silently dropped and graceful shutdown hung. Mirror the epoll engine: register a signalfd as an ordinary multishot-poll read event and set signal_support, so the signal thread is never started and signals are delivered on the ring exactly like fd readiness. The eventfd is retained for cross-thread posts (signo 0), where it is correct. For this to be robust the CQE handler now re-arms any terminated multishot (!IORING_CQE_F_MORE) on a still-armed direction, not only when the completion carried no readiness -- essential for the signalfd, whose handler never calls enable_read -- and the eventfd doorbell re-arms itself the same way. Verified: unitd on the io_uring engine exits cleanly on SIGTERM, and the test_return/test_proxy/test_http_header subset passes without hanging.
…hot poll Multishot IORING_OP_POLL_ADD is edge-like: the kernel posts one CQE per wait queue wakeup, never re-reports a still-ready fd, and does not merge pending readiness into a single state-carrying event the way epoll_wait() does. The Stage-1 engine treated completions as level events and reused the plain nxt_unix_conn_io, which broke three of epoll's guarantees Unit relies on and wedged or crashed the router under proxy keepalive load: 1. Pending-EOF loss (proxy stall). Upstream response and FIN arriving in one wakeup produce a single POLLIN|POLLRDHUP CQE; recv() consuming the data as a short read clears read_ready per the level contract and the EOF is never re-reported. Ship nxt_io_uring_conn_io, a copy of nxt_unix_conn_io whose recvbuf shim forces read_ready back on when data was read with an EOF pending -- the exact analogue of nxt_epoll_edge_conn_io_recvbuf. The CQE handler latches ev->epoll_eof/epoll_error from POLLRDHUP/POLLERR/POLLHUP. The latches are set-only: CQEs are per-wakeup snapshots, so a plain POLLIN CQE can be delivered after the POLLRDHUP one within a single drain and must not erase the pending-EOF mark (observed in the wild; the assignment form lost the FIN and stalled the proxied connection). 2. One handler dispatch per fd and direction per poll (router SIGSEGV). epoll_wait() returns at most one event per fd per cycle and all queued work runs before the next poll, so a handler can never be enqueued twice with the same stale object. One CQ drain, however, can hold several CQEs for the same fd and direction (data and FIN as separate wakeups): both dispatched the read handler, the first run completed and freed the request, and the second ran on freed state -- crashing the router under load (signal 11) and during mass close at shutdown. A per-drain sequence number on the slot now coalesces dispatches exactly like epoll while all readiness latches are still merged from every CQE. 3. Listener re-fire (accept stall, contract item #5). A multishot poll on the listen socket never re-reports a still-non-empty backlog, so a partial accept batch stranded the remaining connections. enable_accept now arms a non-multishot POLL_ADD and the CQE handler re-arms it after every event; POLL_ADD re-checks readiness at submission, firing immediately while the backlog is non-empty, which emulates the required level-triggered re-fire at one SQE per accept batch. Verified with a proxy-to-self keepalive load (oha -c 8 and -c 64): before, double-digit timeout rates and router crashes; after, 3200/3200 requests succeed across 80 rounds with no crashes, and SIGTERM under sustained load shuts down cleanly 10/10 times.
The startup probe cannot guarantee ring creation keeps working for the whole process tree: io_uring resources (ring memory, fds) can run out at any later fork or router worker start, and only the main-process path had a fallback. Observed in practice: io_uring_queue_init_params() returning ENOMEM in a child process killed it and left a half-started instance serving nothing. Move the fallback into nxt_event_engine_create() and nxt_event_engine_change(): when the io_uring interface fails to create its event set, the engine is created with epoll instead of failing the process. This covers all four creation sites (main process, forked children, router workers, facility change) with a single decision point; the per-site retry in nxt_runtime_event_engines() is dropped as redundant. The "using ... event engine" log line moves after the final interface is known and is also emitted by nxt_event_engine_change() so re-created child engines report what they run. NXT_IO_URING_FORCE_TIER=create is a new debug value that passes the probe but fails create(), exercising this fallback end to end: with it set, the whole tree (main, controller, router and its workers) comes up on epoll_edge and serves traffic.
- Mask the slot generation before comparing with a CQE's generation (nxt_iou_gen): the counters are full uint16_t but only GEN_MASK bits travel in user_data, so an unmasked compare starts rejecting every completion once a reused fd number passes 2^14 arm/disarm cycles, permanently stalling that fd. - Drain the signalfd to EAGAIN in the handler: multishot poll delivery is edge-like and the CQ drain dedupes to one handler call, so a single read() could strand a queued signal (e.g. SIGTERM behind SIGCHLD) until an unrelated wakeup. Save errno before nxt_debug() so the EAGAIN check cannot be clobbered by logging. - Track ring initialization separately from the feature tier (ring_inited): setup() can fail between a successful queue_init and the tier assignment (multishot probe), and free() keyed on the tier leaked the ring fd and its mmaps on that epoll-fallback path. - Reject negative fds in nxt_io_uring_slot(): cast to uint32_t a -1 fd passed the grow check and then shrank the slot table to one entry, memzeroing out of bounds. All callers tolerate NULL. - Make the configure feature test compile the liburing >= 2.2 APIs the engine uses (io_uring_prep_poll_multishot, io_uring_sqe_set_data64) so an older liburing fails configure instead of the build. - Replace the loop in nxt_io_uring_pow2() with the branch-free round-up; the loop could not terminate for n > 2^31 (unreachable from current callers, but now impossible by construction). - Log each drained CQE (user_data/res/flags) at debug level, mirroring the epoll engine's per-event debug line; this instrumentation is what root-caused the edge-delivery bugs and costs nothing in non-debug builds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b261dd3 to
a50e1ce
Compare
1 participant
io_uring event engine — Stage 1: poll-mode readiness bridge
Adds an opt-in io_uring event engine to reduce per-connection syscall overhead and improve C10K-class scalability. Stage 1 keeps Unit's readiness model unchanged and uses io_uring as a "better epoll": a multishot
IORING_OP_POLL_ADDper armed direction feeds the existingread_ready/write_readylatches, with allrecv/send/acceptsyscalls staying in userspace. Submit and wait fuse into a singleio_uring_enter(2)per loop; steady-state armed connections cost zero control syscalls.Design highlights
user_data; disable/delete/close bump them so completions for a stale arming (including after fd reuse) are rejected before touching the event object.create()failure at any later point (ring exhaustion in a child, seccomp,kernel.io_uring_disabled) degrades to epoll instead of dying.NXT_IO_URING=0env kill-switch avoids all io_uring syscalls.Build
./configure --io-uring(opt-in; epoll stays default, engine selectable) or--io-uring-default(io_uring first — the two flavors give a clean A/B pair). Linux-only, guarded byNXT_HAVE_IO_URING; requires liburing and kernel ≥5.13 at runtime (probed).Testing
Stage 2 (completion-mode accept, feature tiers, and the review-hardening pass) is stacked on this branch.
🤖 Generated with Claude Code