Use io_uring for socket readiness - #401
Conversation
8429634 to
2434e46
Compare
bneradt
left a comment
There was a problem hiding this comment.
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.
2434e46 to
36dd5e9
Compare
bneradt
left a comment
There was a problem hiding this comment.
Thanks, this addresses essentially everything from the first pass. Confirming
what I verified on 36dd5e9:
- The readiness/timeout race in
process_completionis fixed.completion.res > 0
now takes priority overtimeout_requested. m_deadlinesis amultimapwith an explicit erase on completion, so the
unbounded growth is gone, andtime_until_next_deadlineis no longer a
scanning loop.fail_servicenow 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-devdoes ship/usr/lib/liburing.a, so the portable release build
will not trip the newFATAL_ERROR. - Two dedicated CI jobs now run unconfined with
PV_TEST_REQUIRE_IO_URING, and
the existing jobs pinPV_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.
9274fd9 to
8c5f671
Compare
bneradt
left a comment
There was a problem hiding this comment.
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()meanssubmit_pending()no longer runs completion
callbacks, so no caller can have itsm_pendingentry destroyed underneath
it. Copying theshared_ptrinexpire_deadlinesand snapshotting the
pending set inrequest_shutdownare good defense in depth on top of that. - The
m_failure_reasonrace is closed. The string is written before the
exchange, so any thread that observesPOLLalso observes a complete
reason. PV_TEST_REQUIRE_IO_URINGis out of the production path. The unit test reads
it itself and calls theSocketIoModeoverload, 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 installis 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.
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.
8c5f671 to
ea1ba92
Compare
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.