Add futex-based blocking when main thread waits for IO poll results#4003
Add futex-based blocking when main thread waits for IO poll results#4003asafpamzn wants to merge 1 commit into
Conversation
When IO threads are enabled and the main thread is waiting for poll results from an IO thread, this change uses futex to efficiently block instead of busy-spinning when the pending IO response count is low. The optimization saves 12-23% CPU at low-medium TPS (50K-700K) with no throughput regression. At high TPS (>900K), pending responses are high enough that the main thread spins to minimize latency. Key changes: - Use io_poll_state directly as the futex variable - Add io_poll_waiting flag to skip FUTEX_WAKE syscall during spinning - Add configurable io-poll-block-threshold (default 8) for tuning - Add INFO stats: io_poll_blocked_count, io_poll_spinning_count - Wrapped in #ifdef __linux__ for cross-platform compatibility Signed-off-by: asafpamzn <asafp@amazon.com>
📝 WalkthroughWalkthroughAdds futex-based blocking to the main thread's IO poll wait path on Linux. When pending IO responses are below a new configurable threshold ( ChangesFutex-based IO poll waiting with threshold config
Sequence DiagramsequenceDiagram
participant MainThread
participant getIOThreadPollResults
participant ioThreadPoll as IOThread (ioThreadPoll)
MainThread->>getIOThreadPollResults: call (io_poll_state == AE_IO_STATE_POLL)
getIOThreadPollResults->>getIOThreadPollResults: read pending response count
alt pending < io_poll_block_threshold
getIOThreadPollResults->>getIOThreadPollResults: set io_poll_waiting = 1
getIOThreadPollResults->>getIOThreadPollResults: FUTEX_WAIT_PRIVATE(io_poll_state, short timeout)
getIOThreadPollResults->>getIOThreadPollResults: stat_io_poll_blocked++
else pending >= threshold
getIOThreadPollResults->>getIOThreadPollResults: spin on io_poll_state
getIOThreadPollResults->>getIOThreadPollResults: stat_io_poll_spinning++
end
ioThreadPoll->>ioThreadPoll: set io_poll_state = AE_IO_STATE_DONE
ioThreadPoll->>MainThread: FUTEX_WAKE_PRIVATE (if io_poll_waiting == 1)
MainThread->>MainThread: resume, process poll results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/io_threads.c (1)
718-731: 💤 Low valueNon-Linux platforms silently skip the spin/block decision tracking.
On non-Linux platforms, neither
stat_io_poll_blockednorstat_io_poll_spinningis incremented whenio_poll_state == AE_IO_STATE_POLL. This means the INFO stats will always show 0 on those platforms, which could be confusing for operators. Consider adding a spinning counter increment for non-Linux to maintain consistent stat semantics.💡 Suggested improvement for cross-platform stat consistency
if (io_state == AE_IO_STATE_POLL) { /* IO thread is still processing poll events. */ `#ifdef` __linux__ /* Decide whether to spin or block based on pending response count. */ int pending = getPendingIOResponsesCount(); if (pending < server.io_poll_block_threshold) { /* Low pending count - block on futex to save CPU. */ atomic_store_explicit(&server.io_poll_waiting, 1, memory_order_release); struct timespec ts = {0, 20000}; /* 20µs timeout */ syscall(SYS_futex, &server.io_poll_state, FUTEX_WAIT_PRIVATE, AE_IO_STATE_POLL, &ts, NULL, 0); atomic_store_explicit(&server.io_poll_waiting, 0, memory_order_release); server.stat_io_poll_blocked++; } else { /* High pending count - spin to minimize latency. */ server.stat_io_poll_spinning++; } +#else + /* Non-Linux: always spin, no futex available. */ + server.stat_io_poll_spinning++; `#endif` return 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io_threads.c` around lines 718 - 731, The spin/block decision tracking inside the `#ifdef __linux__` preprocessor block is not executed on non-Linux platforms, causing the INFO stats (stat_io_poll_blocked and stat_io_poll_spinning) to always show 0 on those systems. Add an `#else` clause after the Linux-specific implementation block to handle non-Linux platforms by incrementing the stat_io_poll_spinning counter, ensuring consistent stat semantics and meaningful INFO output across all supported platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/io_threads.c`:
- Around line 245-250: The futex syscall in the io_threads.c file requires a
32-bit integer pointer, but the code passes a pointer to server.io_poll_state
which is of type AeIoState enum. Since the C standard does not guarantee enum
size, add a compile-time assertion to ensure AeIoState is exactly 32 bits. Place
a static_assert statement near the AeIoState enum definition in src/server.h to
verify that sizeof(AeIoState) equals 4 bytes, preventing undefined behavior on
platforms where the enum might be a different size.
---
Nitpick comments:
In `@src/io_threads.c`:
- Around line 718-731: The spin/block decision tracking inside the `#ifdef
__linux__` preprocessor block is not executed on non-Linux platforms, causing
the INFO stats (stat_io_poll_blocked and stat_io_poll_spinning) to always show 0
on those systems. Add an `#else` clause after the Linux-specific implementation
block to handle non-Linux platforms by incrementing the stat_io_poll_spinning
counter, ensuring consistent stat semantics and meaningful INFO output across
all supported platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cc486a2-59a4-4e8f-bba5-8ef8cf8700c4
📒 Files selected for processing (4)
src/config.csrc/io_threads.csrc/server.csrc/server.h
| #ifdef __linux__ | ||
| /* Wake main thread via futex only if it's actually waiting. */ | ||
| if (atomic_load_explicit(&server.io_poll_waiting, memory_order_acquire)) { | ||
| syscall(SYS_futex, &server.io_poll_state, FUTEX_WAKE_PRIVATE, 1, NULL, NULL, 0); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definition of AeIoState to verify it's 32-bit compatible
rg -n 'typedef.*AeIoState|enum.*AeIoState|AeIoState' --type=c --type-add 'c:*.h' -A3 -B1Repository: valkey-io/valkey
Length of output: 772
🏁 Script executed:
sed -n '695,710p' src/server.hRepository: valkey-io/valkey
Length of output: 518
🏁 Script executed:
rg -n 'sizeof.*AeIoState|static_assert.*AeIoState' --type=c --type-add 'c:*.h'Repository: valkey-io/valkey
Length of output: 42
🏁 Script executed:
sed -n '240,255p' src/io_threads.cRepository: valkey-io/valkey
Length of output: 644
🏁 Script executed:
rg -n 'static_assert.*sizeof|_Static_assert' --type=c --type-add 'c:*.h'Repository: valkey-io/valkey
Length of output: 2234
Add static_assert to verify AeIoState is 32-bit for futex compatibility.
The futex syscall operates on a 32-bit integer (uint32_t*). While AeIoState is a plain enum that typically defaults to int (32-bit on most platforms), the C standard does not guarantee enum size. Add a compile-time check to ensure AeIoState is exactly 32 bits before passing it to futex, matching the pattern already used throughout this codebase:
static_assert(sizeof(AeIoState) == 4, "AeIoState must be 32-bit for futex compatibility");Place this assertion near the enum definition in src/server.h to prevent undefined behavior on platforms where the enum might be a different size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/io_threads.c` around lines 245 - 250, The futex syscall in the
io_threads.c file requires a 32-bit integer pointer, but the code passes a
pointer to server.io_poll_state which is of type AeIoState enum. Since the C
standard does not guarantee enum size, add a compile-time assertion to ensure
AeIoState is exactly 32 bits. Place a static_assert statement near the AeIoState
enum definition in src/server.h to verify that sizeof(AeIoState) equals 4 bytes,
preventing undefined behavior on platforms where the enum might be a different
size.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4003 +/- ##
============================================
- Coverage 76.75% 76.74% -0.02%
============================================
Files 162 162
Lines 81017 81031 +14
============================================
- Hits 62187 62184 -3
- Misses 18830 18847 +17
🚀 New features to boost your workflow:
|
|
@valkey-review-bot Please review this PR. Also, please explain if and why using the futex syscall directly is better than using pthread_mutex. Is there a performance difference for this use case? |
|
Reviewed the futex coordination, the wake/wait ordering, and the build (compiles clean on Re: futex vs
The tradeoff is that the raw |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| /* Low pending count - block on futex to save CPU. */ | ||
| atomic_store_explicit(&server.io_poll_waiting, 1, memory_order_release); | ||
| struct timespec ts = {0, 20000}; /* 20µs timeout */ | ||
| syscall(SYS_futex, &server.io_poll_state, FUTEX_WAIT_PRIVATE, AE_IO_STATE_POLL, &ts, NULL, 0); |
There was a problem hiding this comment.
This FUTEX_WAIT runs even when the event loop was entered with AE_DONT_WAIT. The custom-poll branch in aeProcessEvents (ae.c:428-429) calls custompoll(eventLoop) without consulting AE_DONT_WAIT, and processEventsWhileBlocked() drives the loop with AE_DONT_WAIT (networking.c:6443). Since trySendPollJobToIOThreads() runs unconditionally at the top of beforeSleep (server.c:1850), a poll job can be in flight with the custom poll proc installed during a busy-script yield or RDB/AOF load, and this path then sleeps up to 20µs in a loop that is supposed to make progress without waiting.
Gate the block on the recursive flag so the don't-wait path keeps spinning rather than entering the kernel:
| syscall(SYS_futex, &server.io_poll_state, FUTEX_WAIT_PRIVATE, AE_IO_STATE_POLL, &ts, NULL, 0); | |
| int pending = getPendingIOResponsesCount(); | |
| extern int ProcessingEventsWhileBlocked; | |
| if (!ProcessingEventsWhileBlocked && pending < server.io_poll_block_threshold) { |
(declare ProcessingEventsWhileBlocked via the existing extern in a header if you prefer not to repeat it locally.)
| #ifdef __linux__ | ||
| /* Wake main thread via futex only if it's actually waiting. */ | ||
| if (atomic_load_explicit(&server.io_poll_waiting, memory_order_acquire)) { | ||
| syscall(SYS_futex, &server.io_poll_state, FUTEX_WAKE_PRIVATE, 1, NULL, NULL, 0); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
we do not usually do that. we mostly place syscall handling as a macro in config.h
I agree this sounds right. we should be aligned on also on non-linux platforms. |
|
Reviewed the futex coordination, the wait/wake ordering, the config/stat plumbing, and built The substantive concerns are already on the record and I agree with all three, so I won't re-anchor them: the On futex vs. A condvar requires the signaling side to hold the mutex (or accept a lost-wakeup race), so the IO thread's completion path in On non-Linux alignment (per the request) Right now the whole block is No new correctness issues beyond the above; the remaining items are the design/portability points the thread is already converging on. |
I don't think we should preserve a divergence in this kind of behavior between different supported platforms. it is strange that on macos the reported CPU will behave much different than in linux. I think if we change this we should make this change cross all supported platform. It might be fine to apply a portable implementation and lose some tail latency when the throughput is low and we do not have many io tasks pending, but this is something we need to align on and test |
@ranshid We can check if a POSIX fallback for this is performant enough and use it as a fallback, but if not, I think Linux-only is acceptable. Almost only Linux is used in production AFAIK. I do agree though that it'd be surprising to see busy-looping on one platform and not on another. Btw, it appears you triggered another bot review by quoting my message to the bot. Was it intentional? :D |
I might not fully agree. Even if linux is 99% used in production, I think it might be problematic that many developers will get different numbers and behaviors when running on their dev environments. I AM fine with suffering a little latency cost in low TPS (that would probably impact some benchmarking, but I do not expect that is will be very high - need to test it). Also are we sure there are not many FreeBSD/OpenBSD out there? (also they do have _umtx_op/futex(2) intrinsics ...)
let's make it formal: "Can me and your agent go outside to play?" :) |
dvkashapov
left a comment
dvkashapov
left a comment
There was a problem hiding this comment.
I have some doubts about max throughput scenarios: I suspect there may be slight degradation in both latency and RPS because of extra branch, stats increment and atomic ops. In my opinion we need benchmarks for max throughput case before making any decisions
| createIntConfig("cluster-databases", NULL, IMMUTABLE_CONFIG, 1, INT_MAX, server.config_databases_cluster, 1, INTEGER_CONFIG, NULL, NULL), | ||
| createIntConfig("port", NULL, MODIFIABLE_CONFIG, 0, 65535, server.port, 6379, INTEGER_CONFIG, NULL, updatePort), /* TCP port. */ | ||
| createIntConfig("io-threads", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1, IO_THREADS_MAX_NUM, server.io_threads_num, 1, INTEGER_CONFIG, NULL, updateIOThreads), /* Single threaded by default */ | ||
| createIntConfig("io-poll-block-threshold", NULL, MODIFIABLE_CONFIG, 0, 1000, server.io_poll_block_threshold, 8, INTEGER_CONFIG, NULL, NULL), |
There was a problem hiding this comment.
I don't think any operator would want to change this because it's super low level, lets at least make it hidden config
There was a problem hiding this comment.
I'm not sure I agree with this. I believe it can be very useful to tweak it to balance between CPU usage and latency. Our customers are very worried about high CPU usage and they monitor it carefully for every micro service, so they will likely want to lower the time spinning. (We haven't started using I/O threads yet btw, partly due to the spinning.) Spare capacity (CPU, memory, network) is important for handling bursts (and things like full sync during full traffic) in a highly reliable system.
But I'm not sure the trigger is the best one. How did you decide to use the number of pending jobs for this threshold instead of e.g. spinning a fixed number of iterations (say 10000, or configurable) and then going to sleep?
| "io_poll_blocked_count:%lld\r\n", server.stat_io_poll_blocked, | ||
| "io_poll_spinning_count:%lld\r\n", server.stat_io_poll_spinning, |
There was a problem hiding this comment.
And those metrics would not say a lot to a regular user, maybe better to not include them at all?
There was a problem hiding this comment.
mmm... I agree that they can be removed
Though we may want to keep those in order to have tests.
We need to test it somehow.
We will need to find a way to test it
There was a problem hiding this comment.
We can add them under INFO debug section. Those are undocumented and don't need backward compatibility.
| #ifdef __linux__ | ||
| /* Decide whether to spin or block based on pending response count. */ | ||
| int pending = getPendingIOResponsesCount(); | ||
| if (pending < server.io_poll_block_threshold) { | ||
| /* Low pending count - block on futex to save CPU. */ | ||
| atomic_store_explicit(&server.io_poll_waiting, 1, memory_order_release); | ||
| struct timespec ts = {0, 20000}; /* 20µs timeout */ | ||
| syscall(SYS_futex, &server.io_poll_state, FUTEX_WAIT_PRIVATE, AE_IO_STATE_POLL, &ts, NULL, 0); | ||
| atomic_store_explicit(&server.io_poll_waiting, 0, memory_order_release); | ||
| server.stat_io_poll_blocked++; | ||
| } else { | ||
| /* High pending count - spin to minimize latency. */ | ||
| server.stat_io_poll_spinning++; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I'm thinking wether blocking inside the callback changes scheduling? aeProcessEvents() calls the custom poll proc before the aftersleep callback and before processing fired events, so a 20 microsecond timeout can add latency to ready work if the IO poll completion races. Although I don't know how probable is that in the real world case
There was a problem hiding this comment.
Thanks @dvkashapov ,
- Can you please elaborate on the potential race?
- In most workloads the 20us is not something that customers will notice.
Having said the above, please describe scenario and we can further discuss and make changes if required.
We tested it in max throughput scenarios and there is no degradation, there is a single branch and single counter. Thanks again for the feedback, |
|
This is a very nice feature. I like the approach of balancing spinning and sleeping. I agree with @ranshid that we should add a fallback to pthread_cond for non-Linux posix systems. It could be done later but it maybe be good to think about it from the start. I don't think it's possible to use the same atomic variable for spinning and blocking with pthread_cond, but I don't know how much that matters. Probably not much in practice. I think we should look also at the I/O threads and make then too use a balance of spinning and sleeping. The thread that does epoll (or kqueue or similar) can use sleep on this instead of a futex and other threads can wake it by writing to a pipe. To know if and how a particular thread is sleeping, we could use a bit flags in an atomic variable, so that we can set it a bit for WAITING, one for SPINNING, one for sleeping in a futex/pthread_cond and one for sleeping in epoll/kqueue. I looked into how Erlang's runtime does this balance when waiting for a lock or shared resource or similar. It spinns a fixed number of iterations configurable in steps (e.g. very high = 40000, high = 20000, medium = 10000, low = 5000, none = 0) and then falls asleep. I believe the Go runtime does something similar to balance spinning and sleeping in the runtime, e.g. when scheduling go routines among the available threads, or in lockfree queues, locks, etc. |
|
@zuiderkwast , I'm aligned on supporting mutex with cond var on non linux posix systems. This was my first implementation but it caused performance degradation of few percents. Thus, I suggest to use mutex+cond for non linux posix systems. I agree that we should do it from the start for completeness. To complete the picture: I also tried with event FD, but it was not efficient as well. We can take I/O threads as step 2. |
OK, yeah, so we may want to spin longer to avoid performance regression, depending on OS?
OK, good to know. Regarding the 20 microsecond futex timeout, if epoll has been delegated to an IO thread, nothing can happen on the main thread, so we can hang longer on the futex, until an IO thread wakes the main thread? |
5 participants
Summary
Fixes #4001
When IO threads are enabled and the main thread waits for poll results from an IO thread, it currently busy-spins, consuming ~80- 100% CPU even at low throughput. This PR uses Linux futex to efficiently block the main thread when there's little work pending, while preserving the low-latency spinning behavior at high throughput.
Key changes:
io_poll_stateas the futex variable (no extra synchronization needed)io_poll_waitingflag to skipFUTEX_WAKEsyscall when main thread is spinningio-poll-block-threshold(default 8) for tuningio_poll_blocked_count,io_poll_spinning_count#ifdef __linux__for cross-platform compatibilityBenchmark Results - Pipeline 1
Test Configuration
CPU Usage (Main Thread)
Latency
P50 Latency (ms)
P99 Latency (ms)
Throughput
No regression - both baseline and futex achieve ~1M TPS maximum.
Benchmark Results - Pipeline 10
Test Configuration
CPU Usage (Main Thread)
Latency
P50 Latency (ms)
P99 Latency (ms)
Throughput
No regression - No regression - both baseline and futex achieve 1.6M TPS
Notes
How It Works
When
getIOThreadPollResults()finds the IO thread still polling (AE_IO_STATE_POLL):When the IO thread completes polling in
IOThreadPoll():AE_IO_STATE_DONEFUTEX_WAKEifio_poll_waitingflag is set (avoids syscall overhead at high TPS)