| Message ID | 20210209134939.13083-9-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RAM_NORESERVE, MAP_NORESERVE and hostmem "reserve" property | expand |
On Tue, Feb 09, 2021 at 02:49:38PM +0100, David Hildenbrand wrote: > +#define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory" > +static bool map_noreserve_effective(int fd, bool shared) > +{ > +#if defined(__linux__) > + gchar *content = NULL; > + const char *endptr; > + unsigned int tmp; > + > + /* hugetlbfs behaves differently */ > + if (qemu_fd_getpagesize(fd) != qemu_real_host_page_size) { > + return true; > + } > + > + /* only private shared mappings are accounted (ignoring /dev/zero) */ > + if (fd != -1 && shared) { > + return true; > + } > + > + if (g_file_get_contents(OVERCOMMIT_MEMORY_PATH, &content, NULL, NULL) && > + !qemu_strtoui(content, &endptr, 0, &tmp) && > + (!endptr || *endptr == '\n')) { > + if (tmp == 2) { > + error_report("Skipping reservation of swap space is not supported: " > + " \"" OVERCOMMIT_MEMORY_PATH "\" is \"2\""); > + return false; > + } > + return true; > + } > + /* this interface has been around since Linux 2.6 */ > + error_report("Skipping reservation of swap space is not supported: " > + " Could not read: \"" OVERCOMMIT_MEMORY_PATH "\""); > + return false; > +#else > + return true; > +#endif > +} I feel like this helper wants to fail gracefully for some conditions. Could you elaborate one example and attach to the commit log? I'm also wondering whether it would worth to check the global value. Even if overcommit is globally disabled, do we (as an application process) need to care about it? I think the MAP_NORESERVE would simply be silently ignored by the kernel and that seems to be design of it, otherwise would all apps who uses MAP_NORESERVE would need to do similar things too? Thanks,
On 02.03.21 18:51, Peter Xu wrote: > On Tue, Feb 09, 2021 at 02:49:38PM +0100, David Hildenbrand wrote: >> +#define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory" >> +static bool map_noreserve_effective(int fd, bool shared) >> +{ >> +#if defined(__linux__) >> + gchar *content = NULL; >> + const char *endptr; >> + unsigned int tmp; >> + >> + /* hugetlbfs behaves differently */ >> + if (qemu_fd_getpagesize(fd) != qemu_real_host_page_size) { >> + return true; >> + } >> + >> + /* only private shared mappings are accounted (ignoring /dev/zero) */ >> + if (fd != -1 && shared) { >> + return true; >> + } >> + >> + if (g_file_get_contents(OVERCOMMIT_MEMORY_PATH, &content, NULL, NULL) && >> + !qemu_strtoui(content, &endptr, 0, &tmp) && >> + (!endptr || *endptr == '\n')) { >> + if (tmp == 2) { >> + error_report("Skipping reservation of swap space is not supported: " >> + " \"" OVERCOMMIT_MEMORY_PATH "\" is \"2\""); >> + return false; >> + } >> + return true; >> + } >> + /* this interface has been around since Linux 2.6 */ >> + error_report("Skipping reservation of swap space is not supported: " >> + " Could not read: \"" OVERCOMMIT_MEMORY_PATH "\""); >> + return false; >> +#else >> + return true; >> +#endif >> +} > > I feel like this helper wants to fail gracefully for some conditions. Could > you elaborate one example and attach to the commit log? Sure. The case is "/proc/sys/vm/overcommit_memory == 2" (never overcommit) MAP_NORESERVE is without effect and sparse memory regions are somewhat impossible. > > I'm also wondering whether it would worth to check the global value. Even if > overcommit is globally disabled, do we (as an application process) need to care > about it? I think the MAP_NORESERVE would simply be silently ignored by the > kernel and that seems to be design of it, otherwise would all apps who uses > MAP_NORESERVE would need to do similar things too? Right, I want to catch the "gets silently ignored" part, because someone requested "reserved=off" (!default) but does not actually get what he asked for. As one example, glibc manages heaps via: a) Creating a new heap: mmap(PROT_NONE, MAP_NORESERVE) the maximum size, then mprotect(PROT_READ|PROT_WRITE) the initial heap size. Even if MAP_NORESERVE is ignored, only !PROT_NONE memory ever gets committed ("reserve swap space") in Linux. b) Growing the heap via mprotect(PROT_READ|PROT_WRITE) within the existing mmap. This will commit memory in case MAP_NORESERVE got ignored. c) Shrinking the heap ("discard memory") via MADV_DONTNEED *unless* "/proc/sys/vm/overcommit_memory == 2" - the only way to undo mprotect(PROT_READ|PROT_WRITE) and to un-commit memory is by doing a mmap(PROT_NONE, MAP_FIXED) over the problematic region. If you're interested, you can take a look at: malloc/arena.c sysdeps/unix/sysv/linux/malloc-sysdep.h:check_may_shrink_heap() Thanks!
On Tue, Mar 02, 2021 at 08:01:11PM +0100, David Hildenbrand wrote: > On 02.03.21 18:51, Peter Xu wrote: > > On Tue, Feb 09, 2021 at 02:49:38PM +0100, David Hildenbrand wrote: > > > +#define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory" > > > +static bool map_noreserve_effective(int fd, bool shared) > > > +{ > > > +#if defined(__linux__) > > > + gchar *content = NULL; > > > + const char *endptr; > > > + unsigned int tmp; > > > + > > > + /* hugetlbfs behaves differently */ > > > + if (qemu_fd_getpagesize(fd) != qemu_real_host_page_size) { > > > + return true; > > > + } > > > + > > > + /* only private shared mappings are accounted (ignoring /dev/zero) */ > > > + if (fd != -1 && shared) { > > > + return true; > > > + } [1] > > > + > > > + if (g_file_get_contents(OVERCOMMIT_MEMORY_PATH, &content, NULL, NULL) && > > > + !qemu_strtoui(content, &endptr, 0, &tmp) && > > > + (!endptr || *endptr == '\n')) { > > > + if (tmp == 2) { > > > + error_report("Skipping reservation of swap space is not supported: " > > > + " \"" OVERCOMMIT_MEMORY_PATH "\" is \"2\""); > > > + return false; > > > + } > > > + return true; > > > + } > > > + /* this interface has been around since Linux 2.6 */ > > > + error_report("Skipping reservation of swap space is not supported: " > > > + " Could not read: \"" OVERCOMMIT_MEMORY_PATH "\""); > > > + return false; > > > +#else > > > + return true; > > > +#endif > > > +} > > > > I feel like this helper wants to fail gracefully for some conditions. Could > > you elaborate one example and attach to the commit log? > > Sure. The case is "/proc/sys/vm/overcommit_memory == 2" (never overcommit) > > MAP_NORESERVE is without effect and sparse memory regions are somewhat > impossible. > > > > > I'm also wondering whether it would worth to check the global value. Even if > > overcommit is globally disabled, do we (as an application process) need to care > > about it? I think the MAP_NORESERVE would simply be silently ignored by the > > kernel and that seems to be design of it, otherwise would all apps who uses > MAP_NORESERVE would need to do similar things too? > > Right, I want to catch the "gets silently ignored" part, because someone > requested "reserved=off" (!default) but does not actually get what he asked > for. > > As one example, glibc manages heaps via: > > a) Creating a new heap: mmap(PROT_NONE, MAP_NORESERVE) the maximum size, > then mprotect(PROT_READ|PROT_WRITE) the initial heap size. Even if > MAP_NORESERVE is ignored, only !PROT_NONE memory ever gets committed > ("reserve swap space") in Linux. > > b) Growing the heap via mprotect(PROT_READ|PROT_WRITE) within the existing > mmap. This will commit memory in case MAP_NORESERVE got ignored. > > c) Shrinking the heap ("discard memory") via MADV_DONTNEED *unless* > "/proc/sys/vm/overcommit_memory == 2" - the only way to undo > mprotect(PROT_READ|PROT_WRITE) and to un-commit memory is by doing a > mmap(PROT_NONE, MAP_FIXED) over the problematic region. > > If you're interested, you can take a look at: > > malloc/arena.c > sysdeps/unix/sysv/linux/malloc-sysdep.h:check_may_shrink_heap() Thanks for the context. It's interesting to know libc has such special heap operations. Glibc shrinks heap to save memory for the no-over-commit case, however in our case currently we'd like to fail some users using global_overcommit=2 but reserve=off - it means even if we don't fail the user, mmap() could also fail if it's overcommitted. Even if this mmap() didn't fail, it'll fail very easily later on iiuc, right? I think it's fine to have that early failure, it just seems less helpful than what glibc was doing which shrinks active memory for real, meanwhile there seems to encode some very detailed OS information into this helper, so just less charming. Btw above [1] "fd != -1 && shared" looks weird to me. Firstly it'll bypass overcommit_memory==2 check and return true directly, is that right? I thought the global will be meaningful for all memories except hugetlbfs (in do_mmap() of Linux). Meanwhile, I don't see why file-backed share memories is so special too.. From your commit message, I'm not sure whether you wanted to return false instead, however that's still not the case IIUC, since e.g. /dev/shmem still does accounting iiuc, while MAP_NORESERVE will skip it.
On 02.03.21 22:44, Peter Xu wrote: > On Tue, Mar 02, 2021 at 08:01:11PM +0100, David Hildenbrand wrote: >> On 02.03.21 18:51, Peter Xu wrote: >>> On Tue, Feb 09, 2021 at 02:49:38PM +0100, David Hildenbrand wrote: >>>> +#define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory" >>>> +static bool map_noreserve_effective(int fd, bool shared) >>>> +{ >>>> +#if defined(__linux__) >>>> + gchar *content = NULL; >>>> + const char *endptr; >>>> + unsigned int tmp; >>>> + >>>> + /* hugetlbfs behaves differently */ >>>> + if (qemu_fd_getpagesize(fd) != qemu_real_host_page_size) { >>>> + return true; >>>> + } >>>> + >>>> + /* only private shared mappings are accounted (ignoring /dev/zero) */ >>>> + if (fd != -1 && shared) { >>>> + return true; >>>> + } > > [1] > >>>> + >>>> + if (g_file_get_contents(OVERCOMMIT_MEMORY_PATH, &content, NULL, NULL) && >>>> + !qemu_strtoui(content, &endptr, 0, &tmp) && >>>> + (!endptr || *endptr == '\n')) { >>>> + if (tmp == 2) { >>>> + error_report("Skipping reservation of swap space is not supported: " >>>> + " \"" OVERCOMMIT_MEMORY_PATH "\" is \"2\""); >>>> + return false; >>>> + } >>>> + return true; >>>> + } >>>> + /* this interface has been around since Linux 2.6 */ >>>> + error_report("Skipping reservation of swap space is not supported: " >>>> + " Could not read: \"" OVERCOMMIT_MEMORY_PATH "\""); >>>> + return false; >>>> +#else >>>> + return true; >>>> +#endif >>>> +} >>> >>> I feel like this helper wants to fail gracefully for some conditions. Could >>> you elaborate one example and attach to the commit log? >> >> Sure. The case is "/proc/sys/vm/overcommit_memory == 2" (never overcommit) >> >> MAP_NORESERVE is without effect and sparse memory regions are somewhat >> impossible. >> >>> >>> I'm also wondering whether it would worth to check the global value. Even if >>> overcommit is globally disabled, do we (as an application process) need to care >>> about it? I think the MAP_NORESERVE would simply be silently ignored by the >>> kernel and that seems to be design of it, otherwise would all apps who uses > MAP_NORESERVE would need to do similar things too? >> >> Right, I want to catch the "gets silently ignored" part, because someone >> requested "reserved=off" (!default) but does not actually get what he asked >> for. >> >> As one example, glibc manages heaps via: >> >> a) Creating a new heap: mmap(PROT_NONE, MAP_NORESERVE) the maximum size, >> then mprotect(PROT_READ|PROT_WRITE) the initial heap size. Even if >> MAP_NORESERVE is ignored, only !PROT_NONE memory ever gets committed >> ("reserve swap space") in Linux. >> >> b) Growing the heap via mprotect(PROT_READ|PROT_WRITE) within the existing >> mmap. This will commit memory in case MAP_NORESERVE got ignored. >> >> c) Shrinking the heap ("discard memory") via MADV_DONTNEED *unless* >> "/proc/sys/vm/overcommit_memory == 2" - the only way to undo >> mprotect(PROT_READ|PROT_WRITE) and to un-commit memory is by doing a >> mmap(PROT_NONE, MAP_FIXED) over the problematic region. >> >> If you're interested, you can take a look at: >> >> malloc/arena.c >> sysdeps/unix/sysv/linux/malloc-sysdep.h:check_may_shrink_heap() > > Thanks for the context. It's interesting to know libc has such special heap > operations. > > Glibc shrinks heap to save memory for the no-over-commit case, however in our > case currently we'd like to fail some users using global_overcommit=2 but > reserve=off - it means even if we don't fail the user, mmap() could also fail > if it's overcommitted. Even if this mmap() didn't fail, it'll fail very easily > later on iiuc, right? Here is the issue I want to catch. Assume you create a VM with a virtio-mem device that can grow big in the future. You specify reserved=off. mmap() succeeds and you assume the very sparse memory region does not actually commit memory. But it commits all memory in the sparse mmap. Assume you want to create another VM. It just fails although you still have plenty of free memory - because the previous MAP_NORESERVE got ignored. I want to warn the user right away that the configuration is messed up and that "reserved=off" is not effective. For anonymous memory, "reserved=off" will start really being useful when having a way to dynamically reserve swap space. > > I think it's fine to have that early failure, it just seems less helpful than > what glibc was doing which shrinks active memory for real, meanwhile there > seems to encode some very detailed OS information into this helper, so just > less charming. It's not nice, but the messed-up Linux implementation is to blame. Just read the Linux man page of the mmap where there is an explicit link to "/proc/sys/vm/overcommit_memory" for this very reason. > > Btw above [1] "fd != -1 && shared" looks weird to me. As we never have shared anonymous memory (and that's good, because it is super weird) in QEMU, we can simplify to "if (shared) { return true; }" > > Firstly it'll bypass overcommit_memory==2 check and return true directly, is > that right? I thought the global will be meaningful for all memories except > hugetlbfs (in do_mmap() of Linux). See the description in the patch. On basically all (except anonymous shared) shared memory we don't ever reserve swap space. See mmap_region(): if (accountable_mapping(file, vm_flags)) { charged = len >> PAGE_SHIFT; if (security_vm_enough_memory_mm(mm, charged)) return -ENOMEM; vm_flags |= VM_ACCOUNT; } whereby accountable_mapping(): if (file && is_file_hugepages(file)) return 0; return (vm_flags & (VM_NORESERVE | VM_SHARED | VM_WRITE)) == VM_WRITE; in do_mmap(), we only affect if VM_NORESERVE is set or not. And this makes perfect sense: Most* shared mappings don't need swap space because they can just be written back to the original backing storage (file). This is different to private mappings, which cannot be written back to the file - so we need swap space. > > Meanwhile, I don't see why file-backed share memories is so special too.. From Just think about 100000 processes mapping the same file shared. Would you want to reserve 100000 the same swap space for the same file? Although you never* ever really need swap space because you can just writeback to the file instead of swapping. > your commit message, I'm not sure whether you wanted to return false instead, > however that's still not the case IIUC, since e.g. /dev/shmem still does > accounting iiuc, while MAP_NORESERVE will skip it. *except /dev/shmem. It actually needs swap space because the actual file resides in tmpfs->RAM but Linux will never ever commit that memory. Main reason is: because you don't know when/how much to commit. a) When creating/truncating the file we don't know if it's going to be sparse. So how much should we commit? b) When mapping the file, the same comment as above applies: you don't want to reserve per-mapper but instead per-file. You can create and map gigantic memfd/shmem files even with "/proc/sys/vm/overcommit_memory == 2", because we will never ever commit any of that memory. Yes, shmem is dangerous. Shared mappings behave like always having MAP_NORESERVE, thus the "return true;" It's interesting that you also raise this point: I also want to propose dynamic reservation of swap space for shmem in the future. Instead of being per process, this would have to be per file and similar allow to coordinate with the kernel how much memory we are actually intending to use (->commit) such that the kernel can properly account and reject if it wouldn't be possible. If only a single day would have more than 24 hours :)
On Wed, Mar 03, 2021 at 11:14:10AM +0100, David Hildenbrand wrote: > On 02.03.21 22:44, Peter Xu wrote: > > On Tue, Mar 02, 2021 at 08:01:11PM +0100, David Hildenbrand wrote: > > > On 02.03.21 18:51, Peter Xu wrote: > > > > On Tue, Feb 09, 2021 at 02:49:38PM +0100, David Hildenbrand wrote: > > > > > +#define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory" > > > > > +static bool map_noreserve_effective(int fd, bool shared) > > > > > +{ > > > > > +#if defined(__linux__) > > > > > + gchar *content = NULL; > > > > > + const char *endptr; > > > > > + unsigned int tmp; > > > > > + > > > > > + /* hugetlbfs behaves differently */ > > > > > + if (qemu_fd_getpagesize(fd) != qemu_real_host_page_size) { > > > > > + return true; > > > > > + } > > > > > + > > > > > + /* only private shared mappings are accounted (ignoring /dev/zero) */ > > > > > + if (fd != -1 && shared) { > > > > > + return true; > > > > > + } > > > > [1] > > > > > > > + > > > > > + if (g_file_get_contents(OVERCOMMIT_MEMORY_PATH, &content, NULL, NULL) && > > > > > + !qemu_strtoui(content, &endptr, 0, &tmp) && > > > > > + (!endptr || *endptr == '\n')) { > > > > > + if (tmp == 2) { > > > > > + error_report("Skipping reservation of swap space is not supported: " > > > > > + " \"" OVERCOMMIT_MEMORY_PATH "\" is \"2\""); > > > > > + return false; > > > > > + } > > > > > + return true; > > > > > + } > > > > > + /* this interface has been around since Linux 2.6 */ > > > > > + error_report("Skipping reservation of swap space is not supported: " > > > > > + " Could not read: \"" OVERCOMMIT_MEMORY_PATH "\""); > > > > > + return false; > > > > > +#else > > > > > + return true; > > > > > +#endif > > > > > +} > > > > > > > > I feel like this helper wants to fail gracefully for some conditions. Could > > > > you elaborate one example and attach to the commit log? > > > > > > Sure. The case is "/proc/sys/vm/overcommit_memory == 2" (never overcommit) > > > > > > MAP_NORESERVE is without effect and sparse memory regions are somewhat > > > impossible. > > > > > > > > > > > I'm also wondering whether it would worth to check the global value. Even if > > > > overcommit is globally disabled, do we (as an application process) need to care > > > > about it? I think the MAP_NORESERVE would simply be silently ignored by the > > > > kernel and that seems to be design of it, otherwise would all apps who uses > MAP_NORESERVE would need to do similar things too? > > > > > > Right, I want to catch the "gets silently ignored" part, because someone > > > requested "reserved=off" (!default) but does not actually get what he asked > > > for. > > > > > > As one example, glibc manages heaps via: > > > > > > a) Creating a new heap: mmap(PROT_NONE, MAP_NORESERVE) the maximum size, > > > then mprotect(PROT_READ|PROT_WRITE) the initial heap size. Even if > > > MAP_NORESERVE is ignored, only !PROT_NONE memory ever gets committed > > > ("reserve swap space") in Linux. > > > > > > b) Growing the heap via mprotect(PROT_READ|PROT_WRITE) within the existing > > > mmap. This will commit memory in case MAP_NORESERVE got ignored. > > > > > > c) Shrinking the heap ("discard memory") via MADV_DONTNEED *unless* > > > "/proc/sys/vm/overcommit_memory == 2" - the only way to undo > > > mprotect(PROT_READ|PROT_WRITE) and to un-commit memory is by doing a > > > mmap(PROT_NONE, MAP_FIXED) over the problematic region. > > > > > > If you're interested, you can take a look at: > > > > > > malloc/arena.c > > > sysdeps/unix/sysv/linux/malloc-sysdep.h:check_may_shrink_heap() > > > > Thanks for the context. It's interesting to know libc has such special heap > > operations. > > > > Glibc shrinks heap to save memory for the no-over-commit case, however in our > > case currently we'd like to fail some users using global_overcommit=2 but > > reserve=off - it means even if we don't fail the user, mmap() could also fail > > if it's overcommitted. Even if this mmap() didn't fail, it'll fail very easily > > later on iiuc, right? > > Here is the issue I want to catch. > > Assume you create a VM with a virtio-mem device that can grow big in the > future. You specify reserved=off. mmap() succeeds and you assume the very > sparse memory region does not actually commit memory. But it commits all > memory in the sparse mmap. > > Assume you want to create another VM. It just fails although you still have > plenty of free memory - because the previous MAP_NORESERVE got ignored. Right. So I think that's moving the failure earlier. Again I am not sure how much it would help but it should be slightly better than a latter failure. > > I want to warn the user right away that the configuration is messed up and > that "reserved=off" is not effective. > > For anonymous memory, "reserved=off" will start really being useful when > having a way to dynamically reserve swap space. > > > > > I think it's fine to have that early failure, it just seems less helpful than > > what glibc was doing which shrinks active memory for real, meanwhile there > > seems to encode some very detailed OS information into this helper, so just > > less charming. > > It's not nice, but the messed-up Linux implementation is to blame. Just read > the Linux man page of the mmap where there is an explicit link to > "/proc/sys/vm/overcommit_memory" for this very reason. > > > > > Btw above [1] "fd != -1 && shared" looks weird to me. > > As we never have shared anonymous memory (and that's good, because it is > super weird) in QEMU, we can simplify to > > "if (shared) { return true; }" Yes this looks easier to read. Maybe you can assert the fd in the block too if you are pretty sure of it. > > > > > Firstly it'll bypass overcommit_memory==2 check and return true directly, is > > that right? I thought the global will be meaningful for all memories except > > hugetlbfs (in do_mmap() of Linux). > > See the description in the patch. On basically all (except anonymous shared) > shared memory we don't ever reserve swap space. > > See mmap_region(): > > if (accountable_mapping(file, vm_flags)) { > charged = len >> PAGE_SHIFT; > if (security_vm_enough_memory_mm(mm, charged)) > return -ENOMEM; > vm_flags |= VM_ACCOUNT; > } > > whereby > > accountable_mapping(): > > if (file && is_file_hugepages(file)) > return 0; > return (vm_flags & (VM_NORESERVE | VM_SHARED | VM_WRITE)) == VM_WRITE; > > > in do_mmap(), we only affect if VM_NORESERVE is set or not. > > > And this makes perfect sense: Most* shared mappings don't need swap space > because they can just be written back to the original backing storage > (file). This is different to private mappings, which cannot be written back > to the file - so we need swap space. > > > > > Meanwhile, I don't see why file-backed share memories is so special too.. From > > Just think about 100000 processes mapping the same file shared. Would you > want to reserve 100000 the same swap space for the same file? > > Although you never* ever really need swap space because you can just > writeback to the file instead of swapping. > > > your commit message, I'm not sure whether you wanted to return false instead, > > however that's still not the case IIUC, since e.g. /dev/shmem still does > > accounting iiuc, while MAP_NORESERVE will skip it. > > *except /dev/shmem. It actually needs swap space because the actual file > resides in tmpfs->RAM but Linux will never ever commit that memory. > > Main reason is: because you don't know when/how much to commit. > > a) When creating/truncating the file we don't know if it's going to be > sparse. So how much should we commit? > b) When mapping the file, the same comment as above applies: you don't > want to reserve per-mapper but instead per-file. > > You can create and map gigantic memfd/shmem files even with > "/proc/sys/vm/overcommit_memory == 2", because we will never ever commit any > of that memory. Yes, shmem is dangerous. > > Shared mappings behave like always having MAP_NORESERVE, thus the "return > true;" > > It's interesting that you also raise this point: I also want to propose > dynamic reservation of swap space for shmem in the future. Instead of being > per process, this would have to be per file and similar allow to coordinate > with the kernel how much memory we are actually intending to use (->commit) > such that the kernel can properly account and reject if it wouldn't be > possible. If only a single day would have more than 24 hours :) Thanks, all you said looks sane. I can't say I fully understand what you'd like to propose. To me, a sane user of /dev/shm should really fallocate() on that before using, but I could also be wrong. Another trivial suggestion is you can also add these function pointers into the comment of the helper, e.g., accountable_mapping() as you referenced. So it would be easier for people to understand where these code came from.
>> I want to warn the user right away that the configuration is messed up and >> that "reserved=off" is not effective. >> >> For anonymous memory, "reserved=off" will start really being useful when >> having a way to dynamically reserve swap space. >> >>> >>> I think it's fine to have that early failure, it just seems less helpful than >>> what glibc was doing which shrinks active memory for real, meanwhile there >>> seems to encode some very detailed OS information into this helper, so just >>> less charming. >> >> It's not nice, but the messed-up Linux implementation is to blame. Just read >> the Linux man page of the mmap where there is an explicit link to >> "/proc/sys/vm/overcommit_memory" for this very reason. >> >>> >>> Btw above [1] "fd != -1 && shared" looks weird to me. >> >> As we never have shared anonymous memory (and that's good, because it is >> super weird) in QEMU, we can simplify to >> >> "if (shared) { return true; }" > > Yes this looks easier to read. Maybe you can assert the fd in the block too if > you are pretty sure of it. Yes, and I'll also add more comments to clarify. [...] >> It's interesting that you also raise this point: I also want to propose >> dynamic reservation of swap space for shmem in the future. Instead of being >> per process, this would have to be per file and similar allow to coordinate >> with the kernel how much memory we are actually intending to use (->commit) >> such that the kernel can properly account and reject if it wouldn't be >> possible. If only a single day would have more than 24 hours :) > > Thanks, all you said looks sane. > > I can't say I fully understand what you'd like to propose. To me, a sane user > of /dev/shm should really fallocate() on that before using, but I could also be > wrong. Even then, no swap space gets reserved / memory gets accounted; as accounting is a bit expensive we don't want to do that on fault/discard time. It's still a bit of a puzzle. Also, fallocate() gets pretty useless once you bring memory overcommit / virtio-balloon into the picture :( > > Another trivial suggestion is you can also add these function pointers into the > comment of the helper, e.g., accountable_mapping() as you referenced. So it > would be easier for people to understand where these code came from. Yes, I'll try adding more comments.
diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 9820d845c0..f45a85add1 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -2248,6 +2248,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length) abort(); } else { flags = MAP_FIXED; + flags |= (block->flags & RAM_NORESERVE) ? MAP_NORESERVE : 0; if (block->fd >= 0) { flags |= (block->flags & RAM_SHARED ? MAP_SHARED : MAP_PRIVATE); diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c index bb99843106..f77d9ca574 100644 --- a/util/mmap-alloc.c +++ b/util/mmap-alloc.c @@ -20,6 +20,7 @@ #include "qemu/osdep.h" #include "qemu/mmap-alloc.h" #include "qemu/host-utils.h" +#include "qemu/cutils.h" #include "qemu/error-report.h" #define HUGETLBFS_MAGIC 0x958458f6 @@ -120,7 +121,7 @@ static void *mmap_reserve(size_t size, int fd) * it accessible. */ static void *mmap_activate(void *ptr, size_t size, int fd, bool readonly, - bool shared, bool is_pmem) + bool shared, bool is_pmem, bool noreserve) { const int prot = PROT_READ | (readonly ? 0 : PROT_WRITE); int map_sync_flags = 0; @@ -129,6 +130,7 @@ static void *mmap_activate(void *ptr, size_t size, int fd, bool readonly, flags |= fd == -1 ? MAP_ANONYMOUS : 0; flags |= shared ? MAP_SHARED : MAP_PRIVATE; + flags |= noreserve ? MAP_NORESERVE : 0; if (shared && is_pmem) { map_sync_flags = MAP_SYNC | MAP_SHARED_VALIDATE; } @@ -170,6 +172,43 @@ static inline size_t mmap_guard_pagesize(int fd) #endif } +#define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory" +static bool map_noreserve_effective(int fd, bool shared) +{ +#if defined(__linux__) + gchar *content = NULL; + const char *endptr; + unsigned int tmp; + + /* hugetlbfs behaves differently */ + if (qemu_fd_getpagesize(fd) != qemu_real_host_page_size) { + return true; + } + + /* only private shared mappings are accounted (ignoring /dev/zero) */ + if (fd != -1 && shared) { + return true; + } + + if (g_file_get_contents(OVERCOMMIT_MEMORY_PATH, &content, NULL, NULL) && + !qemu_strtoui(content, &endptr, 0, &tmp) && + (!endptr || *endptr == '\n')) { + if (tmp == 2) { + error_report("Skipping reservation of swap space is not supported: " + " \"" OVERCOMMIT_MEMORY_PATH "\" is \"2\""); + return false; + } + return true; + } + /* this interface has been around since Linux 2.6 */ + error_report("Skipping reservation of swap space is not supported: " + " Could not read: \"" OVERCOMMIT_MEMORY_PATH "\""); + return false; +#else + return true; +#endif +} + void *qemu_ram_mmap(int fd, size_t size, size_t align, @@ -182,8 +221,7 @@ void *qemu_ram_mmap(int fd, size_t offset, total; void *ptr, *guardptr; - if (noreserve) { - error_report("Skipping reservation of swap space is not supported"); + if (noreserve && !map_noreserve_effective(fd, shared)) { return MAP_FAILED; } @@ -204,7 +242,8 @@ void *qemu_ram_mmap(int fd, offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr; - ptr = mmap_activate(guardptr + offset, size, fd, readonly, shared, is_pmem); + ptr = mmap_activate(guardptr + offset, size, fd, readonly, shared, is_pmem, + noreserve); if (ptr == MAP_FAILED) { munmap(guardptr, total); return MAP_FAILED;
Let's support RAM_NORESERVE via MAP_NORESERVE. At least on Linux, the flag has no effect on shared mappings - except for anonymous memory and hugetlbfs. Linux man page: "MAP_NORESERVE: Do not reserve swap space for this mapping. When swap space is reserved, one has the guarantee that it is possible to modify the mapping. When swap space is not reserved one might get SIGSEGV upon a write if no physical memory is available. See also the discussion of the file /proc/sys/vm/overcommit_memory in proc(5). In kernels before 2.6, this flag had effect only for private writable mappings." Note that the "guarantee" part is wrong with memory overcommit in Linux. Also, in Linux hugetlbfs is treated differently - we configure reservation of huge pages from the pool, not reservation of swap space (huge pages cannot be swapped). The rough behavior is [1]: a) !Hugetlbfs: 1) Without MAP_NORESERVE *or* with memory overcommit under Linux disabled ("/proc/sys/vm/overcommit_memory == 2"), the following accounting/reservation happens: For a file backed map SHARED or READ-only - 0 cost (the file is the map not swap) PRIVATE WRITABLE - size of mapping per instance For an anonymous or /dev/zero map SHARED - size of mapping PRIVATE READ-only - 0 cost (but of little use) PRIVATE WRITABLE - size of mapping per instance 2) With MAP_NORESERVE, no accounting/reservation happens. b) Hugetlbfs: 1) Without MAP_NORESERVE, huge pages are reserved. 2) With MAP_NORESERVE, no huge pages are reserved. Note: With "/proc/sys/vm/overcommit_memory == 0", we were already able to configure it for !hugetlbfs globally; this toggle now allows configuring it more fine-grained, not for the whole system. The target use case is virtio-mem, which dynamically exposes memory inside a large, sparse memory area to the VM. [1] https://www.kernel.org/doc/Documentation/vm/overcommit-accounting Signed-off-by: David Hildenbrand <david@redhat.com> --- softmmu/physmem.c | 1 + util/mmap-alloc.c | 47 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 4 deletions(-)