Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back DAX #205

Closed
wants to merge 4 commits into from
Closed

Bring back DAX #205

wants to merge 4 commits into from

Conversation

asahilina
Copy link
Contributor

Re-add the shmem region required for DAX again (after it got removed when virtio-gpu was implemented), and enable DAX.

asahilina added 4 commits July 6, 2024 15:15
We will use half for DAX and half for virtio-gpu.

Signed-off-by: Asahi Lina <[email protected]>
With DAX disabled, the shmem region was reused for virtio-gpu. Split it
in half and use one half for DAX and the other half for virtio-gpu.

Signed-off-by: Asahi Lina <[email protected]>
Commit eac9478 ("virtio/fs: move work to a separate thread")
removed support for shmem regions, and therefore DAX. Pass through the
shmem region to the worker to make DAX work again.

Fixes: eac9478 ("virtio/fs: move work to a separate thread")
Signed-off-by: Asahi Lina <[email protected]>
This reverts commit 0baaa7f.

Signed-off-by: Asahi Lina <[email protected]>
@dgageot
Copy link
Contributor

dgageot commented Jul 7, 2024

Hey @asahilina, what's the best way to test that DAX is working again?

@asahilina
Copy link
Contributor Author

asahilina commented Jul 8, 2024 via email

@dgageot
Copy link
Contributor

dgageot commented Jul 8, 2024

The kernel will try to use it unconditionally if the mount flag is present (that's how I figured out what was missing, enabling DAX just broke everything until I got it right). I haven't truly tested it end to end though, but I'll find out soon enough if it's working as advertised since a use case I'm interested in relies on coherent /dev/shm memory mappings between the host and the guest. ^^

Thanks a lot! If I understand correctly, your use case is using DAX on the boot "disk" that's over virtiofs. I'm interested in using DAX on virtiofs mounts for file sharing between the host and the guest and couldn't see a difference with this PR.
But it's more than possible that I understood it wrong or that I'm doing it wrong.

@asahilina
Copy link
Contributor Author

asahilina commented Jul 8, 2024 via email

@dgageot
Copy link
Contributor

dgageot commented Jul 8, 2024

Thank! Yes, that's what I did and in does support the dax mount option now, whereas it didn't before. I didn't see any difference on the performance, though.

intc.clone(),
virgl_flags,
#[cfg(target_os = "macos")]
_map_sender,
)?;
}
#[cfg(not(feature = "tee"))]
attach_fs_devices(&mut vmm, &vm_resources.fs, None, intc.clone())?;
attach_fs_devices(&mut vmm, &vm_resources.fs, dax_shm_region, intc.clone())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the feeling, from what I have tested, that this share memory region can't be used by more than one virtio device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to work fine for me? I don't see why it wouldn't work as long as the range used by each device does not overlap (which is why I split it in half here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the half/half splitting between gpu and fs clearly works.
If the user has two virtiofs devices, they end up sharing the exact same portion, though.
That makes all my virtiofs devices fail to setup, except for the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, sorry, I misunderstood. Maybe @slp knows what the intent was here? I only brought back the previous behavior from back when DAX was enabled, so I'm guessing this same problem already existed before?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to implement some kind of shm region manager that manages the window and can be used to request regions of a certain size. I'm adding this to my TODO list and I think I'll have bandwidth to implement it next week, in case no one beats me implementing it first.

@@ -554,13 +554,27 @@ pub fn build_microvm(
)?;
}

// Use half of the shmem region for virtio-gpu and half for DAX
let _shm_region_size = arch_memory_info.shm_size >> 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

My rust is still very recent: should the underscore be removed, here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That silences the unused variable warning, in the case of a build with the tee feature enabled and the gpu feature disabled (which would cause it to become unused).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the noise!

@slp
Copy link
Contributor

slp commented Sep 11, 2024

This one got superseded by #212

@slp slp closed this Sep 11, 2024
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.

3 participants