Skip to content

Use io_uring for socket readiness - #401

Merged
bneradt merged 1 commit into
yahoo:masterfrom
bneradt:add-io-uring
Sep 3, 2026
Merged

Use io_uring for socket readiness#401
bneradt merged 1 commit into
yahoo:masterfrom
bneradt:add-io-uring

Conversation

@bneradt

@bneradt bneradt commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Use a process-wide io_uring to multiplex socket readiness waits on
supported Linux systems while preserving poll as the automatic fallback.

Add auto, off, and required modes for same-binary production A/B
tests, including fail-fast behavior when a canary cannot keep using
io_uring.

Production validation: compare --io-uring required with
--io-uring off before merging.

Copilot AI lite review requested due to automatic review settings August 21, 2026 23:16
@bneradt bneradt self-assigned this Aug 21, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@bneradt
bneradt force-pushed the add-io-uring branch 2 times, most recently from 8429634 to 2434e46 Compare August 28, 2026 20:07

@bneradt bneradt left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is careful work. The poll fallback is structured so that a
missing or unusable io_uring degrades cleanly, the lifetime handling around
PollOperation looks sound, and I did not find a data race or a leak.

My main reservations are about validation rather than the code itself.

Performance. For this thread-per-connection design, each readiness wait
replaces a single poll() syscall with an eventfd write, a completion-thread
wakeup, a submit syscall, a futex wake, and two context switches, all
serialized through one mutex and one thread. I would expect that to be slower
than poll(2), not faster. The PR description says to compare --io-uring required against --io-uring off before merging. Please post those numbers
here. If there is no measurable win, this is roughly 700 lines of concurrency
code to carry for a switch that stays off.

Coverage. As it stands CI cannot execute the io_uring path at all, so the
new backend is untested in the merge gate. Details in the inline comment on
the unit test.

The rest of the inline comments are one correctness issue, some operational
gaps around diagnosing a failure in required mode, and a few nits.

Comment thread tests/unit_tests/test_socket_io.cc Outdated
Comment thread cmake/IoUring.cmake
Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc
Comment thread src/client/verifier-client.cc
Comment thread src/server/verifier-server.cc

@bneradt bneradt left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this addresses essentially everything from the first pass. Confirming
what I verified on 36dd5e9:

  • The readiness/timeout race in process_completion is fixed. completion.res > 0
    now takes priority over timeout_requested.
  • m_deadlines is a multimap with an explicit erase on completion, so the
    unbounded growth is gone, and time_until_next_deadline is no longer a
    scanning loop.
  • fail_service now logs once with the errno and states whether poll will be
    used, which was the missing piece for diagnosing a canary.
  • The backend is reported through the new configure_socket_io(TextView)
    overload, which also removed the duplicated block in both binaries.
  • Real errnos propagate instead of blanket EIO.
  • The static-liburing check in CMake is there. I checked Alpine 3.24 and
    liburing-dev does ship /usr/lib/liburing.a, so the portable release build
    will not trip the new FATAL_ERROR.
  • Two dedicated CI jobs now run unconfined with PV_TEST_REQUIRE_IO_URING, and
    the existing jobs pin PV_ENABLE_IO_URING=OFF. That closes the coverage gap,
    and running the Uranium tests on the ring as well is more than I asked for.
  • <thread> is included, and <vector> is no longer needed now that the
    priority queue is gone.

One new problem, introduced by the EBUSY fix itself: submit_pending() can
now re-enter completion processing, and two callers hold references or
iterators into m_pending across that call. That is a use-after-free. It needs
CQ overflow plus a full submission queue, so it is unlikely, but the failure
mode is memory corruption on a background thread. Details inline.

There is also a smaller data race on m_failure_reason, and a question about
shipping a test-only environment variable in the production code path.

Still the gating item: the --io-uring required versus --io-uring off
comparison. Now that CI can actually run the ring, this is the one thing left
that decides whether the feature earns its complexity. Please post the numbers.

Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc Outdated
Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc Outdated
Comment thread src/core/socket_io.cc Outdated
Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc
Comment thread .github/workflows/ci.yml Outdated
@bneradt
bneradt force-pushed the add-io-uring branch 3 times, most recently from 9274fd9 to 8c5f671 Compare September 3, 2026 15:26

@bneradt bneradt left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on 8c5f671. Everything from the second pass is resolved, and CI is
green on this head including both io_uring jobs, which confirms the published
image now carries liburing and that --verifier-io-uring required reaches the
Uranium plugin.

  • The reentrancy use-after-free is gone. Splitting reap_completions() from
    drain_completions() means submit_pending() no longer runs completion
    callbacks, so no caller can have its m_pending entry destroyed underneath
    it. Copying the shared_ptr in expire_deadlines and snapshotting the
    pending set in request_shutdown are good defense in depth on top of that.
  • The m_failure_reason race is closed. The string is written before the
    exchange, so any thread that observes POLL also observes a complete
    reason.
  • PV_TEST_REQUIRE_IO_URING is out of the production path. The unit test reads
    it itself and calls the SocketIoMode overload, and the Uranium jobs pass the
    mode as a real command-line flag through the new --verifier-io-uring
    option and the {verifier-io-uring} substitution. Plumbing that through the
    sigint test as well was thorough.
  • The job-time apt-get install is gone now that the image has the package.

One new issue, again a consequence of the previous fix: completions that
reap_completions() parks in m_deferred_completions can be stranded while
run() blocks in io_uring_wait_cqe. Details inline on run(). It is a
one-line fix.

I am satisfied with the code once that is addressed. The benchmark comparing
--io-uring required against --io-uring off is still the open question for
whether to merge at all, and I have not seen numbers yet.

Comment thread src/core/socket_io.cc
Comment thread src/core/socket_io.cc
Use a process-wide io_uring to multiplex socket readiness waits on
supported Linux systems while preserving poll as the automatic fallback.

Add auto, off, and required modes for same-binary production A/B
tests, including fail-fast behavior when a canary cannot keep using
io_uring.

Production validation: compare --io-uring required with
--io-uring off before merging.
@bneradt
bneradt merged commit d3f071f into yahoo:master Sep 3, 2026
6 checks passed
@bneradt
bneradt deleted the add-io-uring branch September 3, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants