Skip to content

Conversation

@mkeeter
Copy link
Contributor

@mkeeter mkeeter commented Jan 24, 2024

This PR is staged on top of #1105 and #1106, so only the last commit (409d225) is new.

At last, we're ready to switch to per-block (instead of per-byte) ownership! This means that buffers take up 50% less RAM, and do significantly less work during reads.

Before (from #1094)

4K READ: bw=52.1MiB/s (54.6MB/s), 52.1MiB/s-52.1MiB/s (54.6MB/s-54.6MB/s), io=3126MiB (3278MB), run=60002-60002msec
1M READ: bw=355MiB/s (372MB/s), 355MiB/s-355MiB/s (372MB/s-372MB/s), io=20.8GiB (22.4GB), run=60080-60080msec
4M READ: bw=352MiB/s (369MB/s), 352MiB/s-352MiB/s (369MB/s-369MB/s), io=20.7GiB (22.2GB), run=60273-60273msec

After

4K READ: bw=50.8MiB/s (53.2MB/s), 50.8MiB/s-50.8MiB/s (53.2MB/s-53.2MB/s), io=3046MiB (3194MB), run=60002-60002msec
1M READ: bw=644MiB/s (675MB/s), 644MiB/s-644MiB/s (675MB/s-675MB/s), io=37.8GiB (40.6GB), run=60038-60038msec
4M READ: bw=576MiB/s (604MB/s), 576MiB/s-576MiB/s (604MB/s-604MB/s), io=33.9GiB (36.4GB), run=60176-60176msec

Here's the Propolis patch to make it work:

diff --git a/lib/propolis/src/block/crucible.rs b/lib/propolis/src/block/crucible.rs
index 3a403e7..233a1e7 100644
--- a/lib/propolis/src/block/crucible.rs
+++ b/lib/propolis/src/block/crucible.rs
@@ -34,6 +34,9 @@ impl WorkerState {
     async fn process_loop(&self, acc_mem: MemAccessor) {
         let read_only = self.info.read_only;
         let skip_flush = self.skip_flush;
+
+        // Build an empty buffer, which will be resized before any operation
+        let mut buffer = Buffer::with_capacity(0, 0);
         loop {
             let req = match self.attachment.wait_for_req().await {
                 Some(r) => r,
@@ -49,6 +52,7 @@ impl WorkerState {
                     skip_flush,
                     &req,
                     &memctx,
+                    &mut buffer,
                 )
                 .await
                 {
@@ -336,6 +340,7 @@ async fn process_request(
     skip_flush: bool,
     req: &block::Request,
     mem: &MemCtx,
+    buffer: &mut Buffer,
 ) -> Result<(), Error> {
     match req.oper() {
         block::Operation::Read(off, len) => {
@@ -343,17 +348,17 @@ async fn process_request(
                 req.mappings(mem).ok_or_else(|| Error::BadGuestRegion)?;

             let offset = block.byte_offset_to_block(off as u64).await?;
+            let bs = block.check_data_size(len).await? as usize;

             // Perform one large read from crucible, and write from data into
             // mappings
-            let data = Buffer::new(len);
-            let _ = block.read(offset, data.clone()).await?;
+            buffer.reset(len / bs, bs);
+            block.read(offset, buffer).await?;

-            let source = data.as_vec().await;
             let mut nwritten = 0;
             for mapping in maps {
                 nwritten += mapping.write_bytes(
-                    &source[nwritten..(nwritten + mapping.len())],
+                    &buffer[nwritten..(nwritten + mapping.len())],
                 )?;
             }

(also includes changes from #1094)

@mkeeter mkeeter requested review from jmpesp, leftwo and pfmooney January 24, 2024 15:30
@leftwo
Copy link
Contributor

leftwo commented Jan 24, 2024

I'll go through this (after the other PRs before it) but when we get close, we should have the Propolis PR also ready so we can roll this all up together. I can help with that too.

@mkeeter mkeeter force-pushed the per-block-ownership-redux branch from 409d225 to 34dde7d Compare January 24, 2024 19:17
@mkeeter
Copy link
Contributor Author

mkeeter commented Jan 24, 2024

Rebased and ready for review!

@mkeeter mkeeter marked this pull request as ready for review January 24, 2024 19:17
Copy link
Contributor

@jmpesp jmpesp left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@pfmooney pfmooney left a comment

Choose a reason for hiding this comment

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

A few potential nits, but this is exciting stuff

@mkeeter mkeeter force-pushed the per-block-ownership-redux branch from 120307c to 8769d72 Compare January 24, 2024 19:55
@mkeeter mkeeter merged commit eb96d24 into oxidecomputer:main Jan 24, 2024
@mkeeter mkeeter deleted the per-block-ownership-redux branch January 24, 2024 20:42
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.

4 participants