Repository navigation
Use SSE2 streaming instructions to improve write throughput - #37
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a WorkerThread streaming_writes flag, replaces per-page writes with a page-range API ChangesNon-temporal Write Path
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
memtouch.cpp (1)
197-197: 💤 Low valueStatic assert message is misleading.
The assertion checks
PAGE_SIZE % (16 * 4)which is divisibility by 64, but the error message states "not a multiple of 16". If this assertion ever fails (unlikely with a constant 4096), the message would be confusing.📝 Suggested fix
- static_assert((PAGE_SIZE % (16 * 4)) == 0, "PAGE SIZE is not a multiple of 16"); + static_assert((PAGE_SIZE % (16 * 4)) == 0, "PAGE_SIZE must be a multiple of 64");🤖 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 `@memtouch.cpp` at line 197, The static_assert currently uses the expression static_assert((PAGE_SIZE % (16 * 4)) == 0, "PAGE SIZE is not a multiple of 16"); which checks divisibility by 64 but the message says 16; update the assertion so the message matches the check (e.g., change the message to "PAGE_SIZE is not a multiple of 64") or change the expression to check modulo 16 if that was intended; locate the static_assert referring to PAGE_SIZE and make the message and expression consistent.
🤖 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.
Nitpick comments:
In `@memtouch.cpp`:
- Line 197: The static_assert currently uses the expression
static_assert((PAGE_SIZE % (16 * 4)) == 0, "PAGE SIZE is not a multiple of 16");
which checks divisibility by 64 but the message says 16; update the assertion so
the message matches the check (e.g., change the message to "PAGE_SIZE is not a
multiple of 64") or change the expression to check modulo 16 if that was
intended; locate the static_assert referring to PAGE_SIZE and make the message
and expression consistent.
phip1611
left a comment
phip1611
left a comment
There was a problem hiding this comment.
Interesting! I still would love to understand a little more what happens on the hardware. What is non-temporal? Why does SSE2 brings the same desired effect as AVX? Why is write increased but read decreased? And ultimatively: Why do we need this at all? I expect to see the same relative effects of vCPU throttling.
Please apologize my ignorance/limited knowledge on this! I'm happy if a simpler solution works for you :)
| .implicit_value(true); | ||
|
|
||
| program.add_argument("--disable_nt") | ||
| .help("Do not provide a non-temporal hint when writing memory pages") |
There was a problem hiding this comment.
Could you please add another sentence? Even I have no idea what a non-temporal hint is :)
Temporal means relating to time. You've probably heard it in other contexts e.g. "Rust provides temporal memory safety". In this context we are talking about when memory is going to be accessed again. The standard assumption is "soon" which is why CPU caches are typically populated with recent writes, possibly leading to older cache entries being evicted (moved up the cache hierarchy to larger slower caches). If you are writing in large (contiguous) batches however, then caching is pure overhead because the cache(s) are filled potentially many times over before you get to the part of the program that is loading/reading the written data. This is why there are instructions to store with a non-temporal hint indicating to the CPU that we do not expect or intend to use this data anytime soon. Such non-temporal stores can thus avoid the overhead of the cache hierarchy and are placed directly into write combining buffers which get issued to main memory at some point in the future, or in our case when they are full. When the write combining buffers are full the CPU needs to flush them to main memory and stalls further writes until that is done. Because filling the buffers is fast, but memory is slow, it doesn't matter that you can fill the buffers (up to) 4x faster with AVX-512 than with SSE2 because flushing to DRAM is the bottleneck. I am not sure exactly what makes the reads slower in this setup as I would have expected the hardware prefetchers to do an excellent job here when reading in a loop and I am a bit surprised myself. I can thus only speculate:
I am not formally educated on these topics and have mainly picked things up here and there. Thus you should take my explanations with a grain of salt. Maybe @tpressure can give a better explanation or correct me if I am (very) wrong about anything here.
If we are able to dirty every single page before a live migration completes with the latest and greatest NICs then we indeed don't need this. If not then maybe we do. I don't know the answer here. When I asked Thomas if |
549cfa5 to
ea964d2
Compare
No need to apologize! We all learn new things every day 🙂 |
very interesting - thanks! That makes sense.
Might be a good question for Werner or Matthias! Thank you! |
ea964d2 to
b54fb68
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
memtouch.cpp (1)
75-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
streaming_writesis silently dropped in both the move constructor and the move assignment operator.The move constructor's member-initializer list (lines 75–85) and the move assignment operator's explicit assignments (lines 87–105) both omit
streaming_writes. In the move constructor, an unlisted scalar member is default-initialized, meaningstreaming_writesis indeterminate in the new object — UB on first read. In the move assignment operator, the field is left untouched, retaining whatever value the destination object already held.The current call site is guarded by
worker_storage.reserve(num_threads)before theemplace_backloop, so no reallocation-triggered move occurs in practice today. That guard is fragile and easy to remove accidentally.🐛 Proposed fix
WorkerThread(WorkerThread&& o) noexcept : id(o.id), run_once(o.run_once), mem_size_mib(o.mem_size_mib), rw_ratio(o.rw_ratio), page_log_ival(o.page_log_ival), + streaming_writes(o.streaming_writes), mem_base(o.mem_base), read_buffer(), stats(std::move(o.stats)) { o.mem_base = nullptr; } WorkerThread& operator=(WorkerThread&& o) noexcept { if (this == &o) { return *this; } cleanup_memory(); id = o.id; run_once = o.run_once; mem_size_mib = o.mem_size_mib; rw_ratio = o.rw_ratio; page_log_ival = o.page_log_ival; + streaming_writes = o.streaming_writes; mem_base = o.mem_base; stats = std::move(o.stats); o.mem_base = nullptr; return *this; }🤖 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 `@memtouch.cpp` around lines 75 - 105, The move constructor WorkerThread(WorkerThread&&) and move-assignment WorkerThread::operator=(WorkerThread&&) are omitting the streaming_writes member, leaving it indeterminate or unchanged; update both to move or copy streaming_writes from the source object (e.g., streaming_writes(o.streaming_writes) in the move ctor initializer list and streaming_writes = o.streaming_writes; in operator=) and then reset o.streaming_writes as appropriate to maintain a valid moved-from state alongside the existing mem_base reset.
🤖 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.
Outside diff comments:
In `@memtouch.cpp`:
- Around line 75-105: The move constructor WorkerThread(WorkerThread&&) and
move-assignment WorkerThread::operator=(WorkerThread&&) are omitting the
streaming_writes member, leaving it indeterminate or unchanged; update both to
move or copy streaming_writes from the source object (e.g.,
streaming_writes(o.streaming_writes) in the move ctor initializer list and
streaming_writes = o.streaming_writes; in operator=) and then reset
o.streaming_writes as appropriate to maintain a valid moved-from state alongside
the existing mem_base reset.
a1a5007 to
a9bddb3
Compare
Using a 100G link and a 30GB (or less) VM: I noticed that if the VM is running memtouch then more iterations are required when live migrating the VM compared to the case when the I could not really see much of a difference in the 1TB VM case though. I am not really sure we need this, but it does make it easier to stress test live migration of smaller VMs. I would advocate for merging this since it doesn't add much code and the non-temporal store behavior can easily be disabled from the command line. @tpressure what do you think? |
|
@olivereanderson looks pretty good to me. The more I think about this, the more I doubt my initial bandwidth calculation. It is ok for 100% read and also for 100% write, but the way I calculate the rate between mixed read and write is bogus. So your results are most certainly correct, it's just a miscalculation. That being said, can we make nt-stores opt-in instead of opt out? I know that a handful of people are using this, so maybe it's a good idea to stay "backward compatible". |
a9bddb3 to
a398794
Compare
Introduce a `write_pages` method that writes a batch of pages and uses 128-bit non-temporal stores on X86_64 CPUs. Replacing the write_pages loops with calls to this function increases write throughput by roughly a factor between 1.7 and 4x depending on the parameters and CPU. We will refactor the main function to use the functionality introduced here in the next commit, but we make non-temporal stores opt-in in case existing use-cases are relying on the current behavior. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
Replace the in-line loops with calls to the new write_pages method, but only apply the non-temporal store optimization if the user has opted in through the new "--nt_stores" argument. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
a398794 to
7786906
Compare
I changed it so that you opt-in via "--nt_stores". |
phip1611
left a comment
phip1611
left a comment
There was a problem hiding this comment.
In commit memtouch: SSE2 version of write_page for x86_64
you could extend the message with "[...] uses 128-bit non-temporal stores (bypassing caches L1, L2, L3) on X86_64 CPUs. [...]"
LGTM!
| return false; | ||
| } | ||
|
|
||
| #if defined(__x86_64__) |
There was a problem hiding this comment.
do we compile this for non x86_64 ever?
There was a problem hiding this comment.
The code is publicly available on Github so there might be someone who does.
Let's just leave it for now.
|
|
||
| program.add_argument("--nt_stores") | ||
| .help( | ||
| "Provide a non-temporal hint when writing memory pages. This can increase write " |
There was a problem hiding this comment.
" a non-temporal hint"
is this really the right wording? In my understanding, you use SIMD-instructions to bypass L1,L2,L3 caches. What is a "hint" there?
There was a problem hiding this comment.
I see your concern, but I think it is the right wording (or at least close to it).
The bit width of the register is an implementation detail and not something the end user needs to be concerned with.
Related: Non-temporal stores, introduced with the SSE2 extension(s), are not SIMD specific and do in fact also exist for 32 bit and 64 bit general purpose registers.
The "hint" is that we do not intend to use the memory shortly after writing to it. There is however no guarantee that the cache is bypassed. This terminology is also used in Intel's SDM.
This PR replaces #35 by solving the same problem, but using the analogous SSE2 (128-bit) instructions instead of AVX-512.
Since every X86_64 processor supports SSE2 we can avoid the complexity of introducing an intermediary static library with different compiler flags.
I have conducted the same benchmarks as in #35 and obtain similar results.
Summary by CodeRabbit