Re: [PATCH v1 8/9] util/mmap-alloc: support RAM_NORESERVE via MAP_NORESERVE

[Peter Xu <peterx@redhat.com>]

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.

-- 
Peter Xu