Add is_hugetlbfs() to GuestMemoryRegion #120

teawater · 2020-11-06T08:07:26Z

Virtio-balloon can release the unused host memory to decrease the memory usage of the VMM.
Release normal pages and hugetlbfs pages require different operations. (madvise MADV_DONTNEED and fallocate64 FALLOC_FL_PUNCH_HOLE)

This commit add Add is_hugetlbfs() to GuestMemoryRegion to help VMM decide if this is a hugetlbfs address or not.

jiangliu · 2020-11-06T08:31:46Z

src/guest_memory.rs

@@ -288,6 +288,11 @@ pub trait GuestMemoryRegion: Bytes<MemoryRegionAddress, E = Error> {
    fn as_volatile_slice(&self) -> Result<volatile_memory::VolatileSlice> {
        self.get_slice(MemoryRegionAddress(0), self.len() as usize)
    }
+
+    /// Returns true if the region is hugepages
+    fn is_hugepages(&self) -> bool {


Does hugepage here mean Hugetlbfs? Or it also includes Transparent Huge Pages?

Yes, hugepage means Hugetlbfs.
THPs can be release as normal pages.

Hugepage is a little ambiguous, should we clearly state that it's hugetlbfs?

jiangliu · 2020-11-06T08:58:04Z

Maybe we lack of some method to attaching some label information to memory regions. For the is_hugepage case, we just store/retrieve a flag associated with a region, and the flag doesn't affect vm-memory behavior. So it would be great to introduce a new common mechanism to attach label data to regions.

teawater · 2020-11-06T09:12:46Z

Maybe we lack of some method to attaching some label information to memory regions. For the is_hugepage case, we just store/retrieve a flag associated with a region, and the flag doesn't affect vm-memory behavior. So it would be great to introduce a new common mechanism to attach label data to regions.

What about add a new u64 flags to MmapRegion to store the label of a region?

teawater · 2020-11-09T02:51:37Z

@jiangliu Pushed a new version according to your comments.

alexandruag · 2020-11-09T12:50:56Z

Hi! Are you using something like memfds for hugepage-backed areas? (that would explain the fallocate). Having a way to express the presence of hugepages at the vm-memory interface level (i.e. GuestMemory and/or GuestMemoryRegion) looks interesting I'm actually not sure what the best approach is (have to think more :D).

However, if you know you're using MmapRegion/GuestRegionMmap it looks like you can inspect the flags member to determine whether MAP_HUGETLB is set (I'm not 100% sure how this interacts with file mappings; would be nice if someone readily has more info here). Also, even at a higher level,MADV_FREE can be applied to any (anonymous) mapping, FALLOC_FL_PUNCH_HOLE to file mappings (if supported by the fs), and a distinction can be made based on the presence or absence of GuestMemoryRegion::file_offset. Is any of this helpful for the time being?

teawater · 2020-11-10T06:37:56Z

Hi! Are you using something like memfds for hugepage-backed areas? (that would explain the fallocate). Having a way to express the presence of hugepages at the vm-memory interface level (i.e. GuestMemory and/or GuestMemoryRegion) looks interesting I'm actually not sure what the best approach is (have to think more :D).

Yes, I am working on virtio-balloon free page reporting of cloud-hypervisor. Its areas without backing file are based on memfds. The areas will setup with MFD_HUGETLB if need.

However, if you know you're using MmapRegion/GuestRegionMmap it looks like you can inspect the flags member to determine whether MAP_HUGETLB is set (I'm not 100% sure how this interacts with file mappings; would be nice if someone readily has more info here).

Thanks! I will use it instead of "set_hugetlbfs" in the next version.

Also, even at a higher level,MADV_FREE can be applied to any (anonymous) mapping, FALLOC_FL_PUNCH_HOLE to file mappings (if supported by the fs), and a distinction can be made based on the presence or absence of GuestMemoryRegion::file_offset. Is any of this helpful for the time being?

I would like to use MADV_FREE as a option but not default.
The reason is:

MADV_FREE is lazy free.
MADV_FREE is just work for anon page.

Best,
Hui

jiangliu · 2020-11-10T09:35:13Z

Hi! Are you using something like memfds for hugepage-backed areas? (that would explain the fallocate). Having a way to express the presence of hugepages at the vm-memory interface level (i.e. GuestMemory and/or GuestMemoryRegion) looks interesting I'm actually not sure what the best approach is (have to think more :D).

Yes, I am working on virtio-balloon free page reporting of cloud-hypervisor. Its areas without backing file are based on memfds. The areas will setup with MFD_HUGETLB if need.

In addition to mmap(MFD_HUGETLB), there are other ways to use hugetlbfs backend. So detecting the MFD_HUGETLB flag may not be concise.

alexandruag · 2020-11-10T12:35:10Z

It's true there are multiple ways to leverage huge pages in general, but specifically within MmapRegion, flags should catch all of them, right? For example, can we mmap a file backed by hugepages without setting that flag?

@teawater, just wondering, why is lazy free worse? Also, with respect to point number 2, you'd use DONT_NEED for anon only as well, since it looks like FALLOC_FL_PUNCH_HOLE has to be used for mmap-ed fds.

That being said, having a high level api to provide page size information sounds interesting. For example I'm wondering whether more information than a simple boolean flag is useful/necessary (i.e. the actual page size). Also, looks like Windows supports something similar but they're called large pages. I would def like to think about his more. @teawater is your use case blocked the existing vm-memory implementation not supporting that information? There's also the possibility of managing an external (with respect to vm_memory) mapping of addresses/regions to huge page information. Can something like that work?

teawater · 2020-11-11T02:26:25Z

It's true there are multiple ways to leverage huge pages in general, but specifically within MmapRegion, flags should catch all of them, right? For example, can we mmap a file backed by hugepages without setting that flag?

Yes.
And memfd with MFD_HUGETLB doesn't need it either.

@teawater, just wondering, why is lazy free worse?

lazy free is not suitable for many environments because the pages will be released when memory pressure.
But memory pressure may cause some performance problems.
Many systems try to avoid going into memory stressed states.
That is why I think MADV_FREE should not be the default operation.

Also, with respect to point number 2, you'd use DONT_NEED for anon only as well, since it looks like FALLOC_FL_PUNCH_HOLE has to be used for mmap-ed fds.

Thanks for your reminding.

That being said, having a high level api to provide page size information sounds interesting. For example I'm wondering whether more information than a simple boolean flag is useful/necessary (i.e. the actual page size). Also, looks like Windows supports something similar but they're called large pages. I would def like to think about his more. @teawater is your use case blocked the existing vm-memory implementation not supporting that information? There's also the possibility of managing an external (with respect to vm_memory) mapping of addresses/regions to huge page information. Can something like that work?

I think it is not about page size but page type.
Linux kernel provides different release pages(keep vma) API for hugetlb pages and others.

alexandruag · 2020-11-12T02:18:32Z

Thanks for the details! I'll have a better look at these things and return to the thread afterwards. Meanwhile, it would be great if other folks chime in as well.

EmeraldShift · 2020-11-16T21:14:38Z

Maybe we lack of some method to attaching some label information to memory regions. For the is_hugepage case, we just store/retrieve a flag associated with a region, and the flag doesn't affect vm-memory behavior. So it would be great to introduce a new common mechanism to attach label data to regions.

This makes sense, but for this specific issue I'm not understanding the purpose behind having set_hugetlbfs, independent of the region constructor. In my mind, it would make more sense for a flag/option to be passed in on region construction that specifies the desired backing behavior (base pages, THP, memfd/hugetlbfs, etc.). That way, the flag (and is_hugetlbfs) would always be consistent with the region's behavior, and we could combine this PR's API (an indicator of the region's hugepage status) with #118 to provide a configurable hugepage API to VMMs.

jiangliu · 2020-11-17T01:29:24Z

Maybe we lack of some method to attaching some label information to memory regions. For the is_hugepage case, we just store/retrieve a flag associated with a region, and the flag doesn't affect vm-memory behavior. So it would be great to introduce a new common mechanism to attach label data to regions.

This makes sense, but for this specific issue I'm not understanding the purpose behind having set_hugetlbfs, independent of the region constructor. In my mind, it would make more sense for a flag/option to be passed in on region construction that specifies the desired backing behavior (base pages, THP, memfd/hugetlbfs, etc.). That way, the flag (and is_hugetlbfs) would always be consistent with the region's behavior, and we could combine this PR's API (an indicator of the region's hugepage status) with #118 to provide a configurable hugepage API to VMMs.

Our current solution is to create file descriptors by the vmm and build memory regions by using the MmapRegion::build() interface. In other words, we have moved the dirty work into vmm:)

alexandruag · 2020-12-01T19:22:05Z

Hi again! Seems like the current iteration of the PR is the most straightforward thing we can do right now to provide the desired functionality. Like @EmeraldShift mentioned, the set method looks a bit out of place, but it's prob the best option until we come up with a new iteration of the interfaces.

Just to clarify the semantics here a bit, if a region is backed by transparent huge pages, the is_hugepages method as defined here should return false right? Would be nice to have a clearer description of the expected behaviour as part of the documentation. Also, does anyone know more about any potential correspondence between huge pages on Linux and large pages on Windows? Should we only present this functionality on platforms where it's relevant (i.e. via conditional compilation options)?

teawater · 2020-12-04T08:59:35Z

Hi again! Seems like the current iteration of the PR is the most straightforward thing we can do right now to provide the desired functionality. Like @EmeraldShift mentioned, the set method looks a bit out of place, but it's prob the best option until we come up with a new iteration of the interfaces.

Just to clarify the semantics here a bit, if a region is backed by transparent huge pages, the is_hugepages method as defined here should return false right?

Yes.

Would be nice to have a clearer description of the expected behaviour as part of the documentation.

What about add the description to https://github.com/rust-vmm/vm-memory/blob/master/DESIGN.md#backend-implementation-based-on-mmap ?

Also, does anyone know more about any potential correspondence between huge pages on Linux and large pages on Windows? Should we only present this functionality on platforms where it's relevant (i.e. via conditional compilation options)?

alexandruag · 2020-12-09T16:21:36Z

Sounds good!

alxiord · 2020-12-15T10:32:35Z

Hi @teawater, we're having some trouble with the CI, so builds might be failing or hanging. We' re sorry for the inconvenience, we'll update you here once it's back up and running.

teawater · 2020-12-15T14:32:42Z

Hi @teawater, we're having some trouble with the CI, so builds might be failing or hanging. We' re sorry for the inconvenience, we'll update you here once it's back up and running.

@aghecenco OK. Thanks!

alexandruag

Hi everyone and apologies for the delay in circling back. Left two more comments that I recently thought about; would be great to hear your opinion on them. After we have a conclusion, we should also add a changelog entry for the additions and decide whether we want the is_hugetlbs method to be conditionally compiled/included depending on the OS, and we're good to go.

alexandruag · 2021-01-05T18:19:51Z

DESIGN.md

@@ -122,6 +122,18 @@ let buf = &mut [0u8; 5];
 let result = guest_memory_mmap.write(buf, addr);
 ```

+One of the responsibilities of `GuestRegionMmap` is to provide an API


On second thought, it's probably best to move something like this description and example to the doc comments of the is_hugetlbfs method from the GuestMemory interface, where it gets better visibility.

alexandruag · 2021-01-05T18:24:04Z

src/guest_memory.rs

@@ -288,6 +288,11 @@ pub trait GuestMemoryRegion: Bytes<MemoryRegionAddress, E = Error> {
    fn as_volatile_slice(&self) -> Result<volatile_memory::VolatileSlice> {
        self.get_slice(MemoryRegionAddress(0), self.len() as usize)
    }
+
+    /// Returns true if the region is backed by hugetlbfs
+    fn is_hugetlbfs(&self) -> bool {


Hmm, would it actually make sense to return an Option<bool>, where None represents that no information is available? In our particular situation (where is_hugetlbfs has to be set via the set_hugetlbfs accesor), this would also be a natural fit since the field starts out as None, and only becomes true or false if it's explicitly set. Just wondering what other folks think about this approach.

alexandruag · 2021-01-21T09:03:55Z

src/mmap_unix.rs

@@ -172,6 +173,7 @@ impl MmapRegion {
            prot,
            flags,
            owned: true,
+            hugetlbfs: false,


Can we have the hugetlbfs field of MmapRegion as an Option<bool> which is None initially? (since there's no available information right now, until we explicitly set it with set_hugetlbfs)

andreeaflorescu · 2021-01-21T12:30:49Z

src/guest_memory.rs

+    /// # Examples
+    ///
+    /// ```
+    /// # #[cfg(feature = "backend-mmap")]


We just merged a PR that updated all the code examples that needed features. Can you please update this example to follow that template?

Changes that would be needed:

specify in the header that this example is based on the the mmap feature. Here is an example:

vm-memory/src/guest_memory.rs

Line 266 in 576a406

/// # Examples (uses the `backend-mmap` feature)

declare the [cfg(feature = "backend-mmap")] only one at the begginging of the example, and get rid of the test_guest_memory_mmap_is_hugetlbfs function. The code can sit directly in the example.

The test_guest_memory_mmap_is_hugetlbs function is still there. To get rid of it, and also declare the #[cfg(feature = "backend-mmap")], we would need to start a code block (and have it all conditionally compiled only for that feature).

/// # #[cfg(feature = "backend-mmap")] /// # { /// # use vm_memory::{GuestAddress, GuestMemory, GuestMemoryMmap, GuestRegionMmap}; /// /// let addr = GuestAddress(0x1000); /// let mem = GuestMemoryMmap::from_ranges(&[(addr, 0x1000)]).unwrap(); /// let r = mem.find_region(addr).unwrap(); /// assert_eq!(r.is_hugetlbfs(), None); /// # }

With this approach, you no longer need the function test_guest_memory_mmap_is_hugetlbfs, and all the cfg macros.

cargo fmt fails because of alignment. The code needs to be aligned as in my previous comment. We can fix it in a subsequent PR.

src/mmap_windows.rs

Virtio-balloon can release the unused host memory to decrease the memory usage of the VMM. Release normal pages and hugetlbfs pages requiring different operations. (madvise MADV_DONTNEED and fallocate64 FALLOC_FL_PUNCH_HOLE) This commit add Add is_hugetlbfs() to GuestMemoryRegion to help VMM decide if this is a hugetlbfs address or not. It returns None represents that no information is available. Signed-off-by: Hui Zhu <[email protected]>

elmarco · 2021-01-27T07:25:57Z

src/guest_memory.rs

+
+    #[cfg(feature = "backend-mmap")]
+    #[test]
+    fn test_guest_memory_mmap_is_hugetlbfs() {


Duplicates the test in comment. (as pointed out before)

elmarco · 2021-01-27T07:43:57Z

Why not add a is_hugetlbs() to GuestRegionMmap and query the underlying MmapRegion flags?

andreeaflorescu · 2021-01-27T08:04:08Z

src/guest_memory.rs

+    /// # Examples
+    ///
+    /// ```
+    /// # #[cfg(feature = "backend-mmap")]


cargo fmt fails because of alignment. The code needs to be aligned as in my previous comment. We can fix it in a subsequent PR.

andreeaflorescu · 2021-01-27T08:07:40Z

src/guest_memory.rs

+    ///     assert_eq!(r.is_hugetlbfs(), None);
+    /// # }
+    /// ```
+    fn is_hugetlbfs(&self) -> Option<bool> {


Sorry for the late comment, but can we actually just use conditional compilation here and make it available only on Linux, instead of having a blank implementation for Windows?

andreeaflorescu · 2021-01-27T08:08:58Z

Why not add a is_hugetlbs() to GuestRegionMmap and query the underlying MmapRegion flags?

I think it might makes sense to have it part of the interface in case this information is needed by crates that do not take a dependency on the mmap backend feature.

elmarco · 2021-01-27T08:17:04Z

Why not add a is_hugetlbs() to GuestRegionMmap and query the underlying MmapRegion flags?

I think it might makes sense to have it part of the interface in case this information is needed by crates that do not take a dependency on the mmap backend feature.

I see. But can't we remove the need for set_hugetlbfs() and query the underlying memory instead? (via some trait)

andreeaflorescu · 2021-01-27T08:39:25Z

I see. But can't we remove the need for set_hugetlbfs() and query the underlying memory instead? (via some trait)

This is an interesting point, that would make the implementation nicer. I do not really know how could we achieve this, so any ideas would be welcome. @bonzini @jiangliu @alexandruag do you have a suggestion?

jiangliu · 2021-01-28T17:03:57Z

I see. But can't we remove the need for set_hugetlbfs() and query the underlying memory instead? (via some trait)

This is an interesting point, that would make the implementation nicer. I do not really know how could we achieve this, so any ideas would be welcome. @bonzini @jiangliu @alexandruag do you have a suggestion?

I don't know any easy way to figure out whether a fd is backed by hugetlbfs:(
We could get file name from /proc/fd/[fdnum], then check the filename. But that's too complicated.

elmarco · 2021-01-28T19:44:46Z

I see. But can't we remove the need for set_hugetlbfs() and query the underlying memory instead? (via some trait)

This is an interesting point, that would make the implementation nicer. I do not really know how could we achieve this, so any ideas would be welcome. @bonzini @jiangliu @alexandruag do you have a suggestion?

I don't know any easy way to figure out whether a fd is backed by hugetlbfs:(
We could get file name from /proc/fd/[fdnum], then check the filename. But that's too complicated.

I was thinking of simply relying on the flags that were given to mmap().

jiangliu · 2021-01-29T01:53:33Z

I see. But can't we remove the need for set_hugetlbfs() and query the underlying memory instead? (via some trait)

This is an interesting point, that would make the implementation nicer. I do not really know how could we achieve this, so any ideas would be welcome. @bonzini @jiangliu @alexandruag do you have a suggestion?

I don't know any easy way to figure out whether a fd is backed by hugetlbfs:(
We could get file name from /proc/fd/[fdnum], then check the filename. But that's too complicated.

I was thinking of simply relying on the flags that were given to mmap().

It works for anonymous memory backed by hugettlbfs by detecting the MAP_HUGETLB flag, but it doesn't work when passing a prepared hugetlbfs fd to MmapRegion::build().

andreeaflorescu

Since this is a long standing PR, let's just merge it, and submit a PR with the required changes afterwards.

teawater requested review from alexandruag, bonzini and jiangliu as code owners November 6, 2020 08:07

jiangliu reviewed Nov 6, 2020

View reviewed changes

teawater force-pushed the memory_huge branch from c1a81f8 to fa424ca Compare November 6, 2020 09:07

teawater force-pushed the memory_huge branch 4 times, most recently from 98d0b34 to 0354dcf Compare November 9, 2020 02:36

teawater changed the title ~~Add get_host_address_and_hugepages() to GuestMemory~~ Add is_hugetlbfs() to GuestMemoryRegion Nov 9, 2020

teawater force-pushed the memory_huge branch from 0354dcf to 9428858 Compare November 10, 2020 08:33

teawater force-pushed the memory_huge branch from 9428858 to bf4d32d Compare November 10, 2020 09:50

teawater mentioned this pull request Nov 13, 2020

virtio-mem: Just call fallocate64 with hugepages cloud-hypervisor/cloud-hypervisor#1909

Closed

jiangliu mentioned this pull request Nov 16, 2020

Support MmapRegion with huge pages #118

Open

nmanthey mentioned this pull request Nov 19, 2020

mmap: try THP via madvise #113

Open

teawater force-pushed the memory_huge branch from bdc8d2d to f05afa9 Compare December 4, 2020 08:58

jiangliu previously approved these changes Dec 10, 2020

View reviewed changes

alexandruag reviewed Jan 5, 2021

View reviewed changes

teawater dismissed jiangliu’s stale review via c5d5b8c January 15, 2021 08:16

teawater force-pushed the memory_huge branch 3 times, most recently from 6ff201f to 1dd3577 Compare January 18, 2021 01:25

alexandruag reviewed Jan 21, 2021

View reviewed changes

teawater force-pushed the memory_huge branch from 1dd3577 to f1f961b Compare January 21, 2021 09:45

jiangliu previously approved these changes Jan 21, 2021

View reviewed changes

andreeaflorescu reviewed Jan 21, 2021

View reviewed changes

teawater dismissed jiangliu’s stale review via 346c1e9 January 22, 2021 04:12

teawater force-pushed the memory_huge branch from f813e7e to 346c1e9 Compare January 22, 2021 04:12

jiangliu previously approved these changes Jan 22, 2021

View reviewed changes

teawater dismissed jiangliu’s stale review via 4728dfd January 22, 2021 08:03

teawater force-pushed the memory_huge branch from 346c1e9 to 4728dfd Compare January 22, 2021 08:03

elmarco reviewed Jan 27, 2021

View reviewed changes

andreeaflorescu reviewed Jan 27, 2021

View reviewed changes

andreeaflorescu approved these changes Feb 4, 2021

View reviewed changes

jiangliu approved these changes Feb 4, 2021

View reviewed changes

jiangliu merged commit e63914e into rust-vmm:master Feb 4, 2021

Add is_hugetlbfs() to GuestMemoryRegion #120

Add is_hugetlbfs() to GuestMemoryRegion #120

Conversation

teawater commented Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu commented Nov 6, 2020

teawater commented Nov 6, 2020

teawater commented Nov 9, 2020

alexandruag commented Nov 9, 2020

teawater commented Nov 10, 2020

jiangliu commented Nov 10, 2020 • edited Loading

alexandruag commented Nov 10, 2020

teawater commented Nov 11, 2020

alexandruag commented Nov 12, 2020

EmeraldShift commented Nov 16, 2020

jiangliu commented Nov 17, 2020

alexandruag commented Dec 1, 2020

teawater commented Dec 4, 2020 • edited Loading

alexandruag commented Dec 9, 2020

alxiord commented Dec 15, 2020

teawater commented Dec 15, 2020

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmarco commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeaflorescu commented Jan 27, 2021

elmarco commented Jan 27, 2021

andreeaflorescu commented Jan 27, 2021

jiangliu commented Jan 28, 2021

elmarco commented Jan 28, 2021

jiangliu commented Jan 29, 2021

andreeaflorescu left a comment

Choose a reason for hiding this comment

teawater commented Nov 6, 2020 •

edited

Loading

jiangliu commented Nov 10, 2020 •

edited

Loading

teawater commented Dec 4, 2020 •

edited

Loading