[PATCH] D96626: Support: mapped_file_region: Pass MAP_NORESERVE to mmap

Fri Mar 19 13:55:06 PDT 2021

JosephTremoulet added a comment.

Thanks for taking a look!

In D96626#2631867 <https://reviews.llvm.org/D96626#2631867>, @sammccall wrote:

> Reading the docs, my understanding is MAP_NORESERVE on file-based mappings:
>
> - is only relevant for writable mappings, read-only mappings should not fail on account of swap space in either case
> - allows the mapping to succeed if there's not enough swap space to write it all, but if you actually write to too much of it, it will segfault
>
> Is that about right?

I think that's close enough (and I'm no expert).  The language I see on the man page is this:

>   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.

So the only error it mentions as a possibility is indeed "upon a write", but then at the end it says "In kernels before 2.6, this flag had effect only for private writable mappings".  I don't know what changed in 2.6, if it now has effect for shared mappings, some esoteric read-only case, or what.  But doing a simple experiment on the Linux machine I have handy (Ubuntu 18.04), yes I'm seeing success for a readonly mapping regardless of MAP_NORESERVE, and success or failure depending on the flag for a read/write mapping.

> If so, the risk is we're turning a handleable error_code into a crash. This might be the right tradeoff for a load coredump in LLDB (where we're very likely to open a huge file RW and then modify very little of it), but seems like a regression for other callers like FileOutputBuffer::create, in turn called by the LLD elf linker with a large filesize.
> So this seems like it might not be safe in general, but if it were an option LLD might specify it.
>
> Am I missing something?

No, I don't think you're missing anything.  But I'll note that the "handleable error_code" case is platform-specific (i.e. regardless of MAP_NORESERVE you'll get the later segfault behavior on NetBSD, FreeBSD, and Windows, IIUC) and so perhaps a bit of false comfort.  Also that without MAP_NORESERVE, whether the error code is returned here depends on how the user's system is configured wrt memory_overcommit.  And lastly that arguably the segfault/AV case could be handled somewhat gracefully, albeit with a lot of effort, by LLD or whatever, using a signal handler / SEH (though of course the prompt failure would be way better for any callers wanting to handle this gracefully).

> (Re: Windows, my understanding matches yours, that we've got non-reserving behavior right now. But that doesn't tell us whether we consistency would be better achieved by adding SEC_COMMIT to the windows version. Confusingly, windows seems to use "commit" to refer to what unix calls "reserve", and SEC_RESERVE is something else).

Sadly, no, SEC_COMMIT doesn't seem to be useful here.  The docs[1] have this to say:

> If the file mapping object is backed by the operating system paging file (the hfile parameter is INVALID_HANDLE_VALUE), specifies that ...
> This attribute has no effect for file mapping objects that are backed by executable image files or data files (the hfile parameter is a handle to a file).

And my local experiments indeed seem to bear that out.

So, while I certainly agree that a prompt failure is easier to handle gracefully, ISTM that making that work on NetBSD or FreeBSD or Windows would require writing our own check of the file size against available memory.  And I don't know if we want to do that or if any of the code using this utility is actively expecting that guarantee (which, again, is only a "guarantee" if the system is configured thus).

Thoughts?

1 - https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createfilemappinga

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96626/new/

https://reviews.llvm.org/D96626