libgap memory allocation on Cygwin

[embray]

As discussed in #27213, when we initialize libgap, we pass it the -s flag which according to the docs is 'set the initially mapped virtual memory', to a size determined by sage.interfaces.gap.get_gap_memory_pool_size(), which on my system happens to be ~3GB.

This is fine, in general, as it's an amount that's available to my system. Nonetheless, in most usage (especially e.g. in the test suite) most of this will not be used.

I have to look into exactly how this memory gets allocated, but however it's being allocated it is unfortunately "committed" memory, meaning that although those pages are allocated lazily, they are guaranteed by the system to be made available for that process, so regardless whether those pages are actually in RAM, space is still reserved for them. So perhaps it uses sbrk to expand the process's heap. When the process is forked, Cygwin's fork() has to copy the parent process's heap into the child. This has the unfortunate effect of accessing those pages, causing them to become allocated in physical memory (even, apparently, if they're clean / unused).

This is essentially the same issue we faced with PARI in #22633, but applied to GAP. In fact, GAP's code sets the default value of the -s flag to 0 on Cygwin, presumably for reasons related to this. This might be possible to avoid, as was done in PARI, by instead allocating this memory with mmap and MAP_NORESERVE.

Upstream PR: gap-system/gap#3335

Depends on #27070
Depends on #27490

Upstream: Fixed upstream, but not in a stable release.

Component: porting: Cygwin

Author: Erik Bray

Branch/Commit: 4b359de

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/27214

[embray]

comment:1

I think this is even sort of implicitly acknowledged in the GAP source code where it sets the default SyAllocPool = 0 if some macro SYS_IS_CYGWIN32 is defined (from the commit history it's hard to tell exactly why that was added, or if any consideration has been given whether the same issue affects 64-bit Cygwin).

I found that as a short term workaround passing -s 0 when initializing libgap on Cygwin is good enough. In that case it will allocate memory as-needed, which may have some performance impact when allocating lots of GAP objects, but for most usage I don't know that it will be noticeable (I'm not sure how best to test this--write a test that allocates lots of GAP objects?)

[jdemeyer]

comment:2

Replying to @embray:

I found that as a short term workaround passing -s 0 when initializing libgap on Cygwin is good enough.

That makes me wonder... if it's good enough for Cygwin, would it be good enough to do that on all systems unconditionally?

[embray]

comment:3

I don't know. The default on 64-bit systems is SyAllocPool = 4096L*1024*1024; /* Note this is in bytes! */ or 4GB. Our routine for determining get_gap_memory_pool_size() actually gives less than this on my system (which has 32GB). I don't really fully understand what the impact is of this in general, though I assumed there were good reasons for how we do it currently.

It's also a question of what is meant by "good enough". Like I wrote, this likely does have impact when allocating a number of large GAP objects; or at least it ensures that there is enough memory reserved for GAP. But I don't really have a good way at hand to measure that. Might have to dig through the history to see what motivated this in the first place.

For my purposes "good enough" is "it works, and most users will never tell the difference either way".

[embray]

comment:4

The current use of -s in the libgap interface was actually introduced by #13588, apparently in order to reduce it, and not out of any performance consideration.

[embray]

comment:5

Okay, I think I misunderstood exactly what -s does and how it impacts memory allocation in GAP (it doesn't help that a lot of documentation about this is out of date / inaccurate).

In fact it only uses the manual sbrk() calls in the specific case of -s 0. Otherwise, for -s > 0 (note: the internal variable that corresponds with -s is called SyAllocPool) it bypasses the manual sbrk() manipulations (thankfully) and instead allocates its own reserved pool for GAP objects, either using mmap (but only, for some reason or other, if madvise is available) or just one big calloc() call.

Cygwin does have madvise so I guess GAP is using mmap here. I think the trick to prevent this behavior is to pass MMAP_NORESERVE when making a big mmap on Cygwin, as this has the effect of preventing the MEM_COMMIT flag from being passed to the underlying VirtualAlloc call. This has an effect similar to overcommitting memory on Linux.

[embray]

comment:6

Replying to @jdemeyer:

Replying to @embray:

I found that as a short term workaround passing -s 0 when initializing libgap on Cygwin is good enough.

That makes me wonder... if it's good enough for Cygwin, would it be good enough to do that on all systems unconditionally?

So to answer your question, no that would not be a good idea on any systems, really.

[embray]

comment:7

This issue is actually pretty bad I think. It doesn't immediately break anything, but it can become a real drag when doing anything that involves forking a lot of new processes, and in particular it can be bad when running the doctests in parallel.

[embray]

comment:8

What I do wonder is what changed that I'm only starting to notice this now. Before upgrading to GAP 4.10, we were still passing the same flags to Volker's libGAP, and I'm pretty sure it would have had the same behavior w.r.t. memory allocation.

Maybe just since we've merged some other changes to make more use of the libGAP interface the problem has become more pronounced. I'd have to install an older version of Sage to compare.

[embray]

comment:9

I'm actually moving this up to blocker: I cannot, even with all other existing fixes applied, get the tests for sage.interfaces.gap to pass. Somewhere early on in that module's tests it invokes libgap. Then later, when it forks to start GAP subprocesses all of them are huge in memory usage. Something about the nature of the tests in this module are such that it forks enough subprocesses, all with the weight of this GAP memory allocation, that it eats up all 32GB of my system's memory and eventually crashes.

I'm still a little unclear as to why this has gotten so bad only with the recent GAP updates, but at least there are a couple possible workarounds, which I'll try shortly...

[embray]

comment:10

I tried applying a patch for GAP (just on Cygwin) to use MAP_NORESERVE when mmap-ing the GAP pool. When I run GAP directly, or even when I run the GAP API tests (in the GAP sources, that is) it appears to work fine, at least superficially.

However, when I use the libgap interface in Sage with this patch, it immediately segfaults:

$ ./sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 8.7.beta4, Release Date: 2019-02-15               │
│ Using Python 2.7.15. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: libgap(1)
------------------------------------------------------------------------
An error occurred during signal handling.
This probably occurred because a *compiled* module has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Python will now terminate.
------------------------------------------------------------------------
Segmentation fault (core dumped)

which is completely bizarre and I can't explain it. I will dig in a bit to see what is happening here...

[embray]

comment:11

I see: When you create an mmap with MAP_NORESERVE on Cygwin, if you try to access the allocated region it creates without committing it results in an access violation--you can't read or write to that memory until it's been explicitly committed with an additional VirtualAlloc call.

Normally Cygwin handles this transparently: when its vectored exception handler catches a STATUS_ACCESS_VIOLATION it first checks if the access violation was on a memory address inside an mmap created with MAP_NORESERVE. If so, it the commits the page containing the address in question, and if that is successful the exception handler returns a special value (understood by the kernel ExceptionContinueExecution) which basically means the error has been handled, and it should retry the operation (in this case the memory access) that resulted in the exception.

This works fine normally, which is why I don't see any problem when running GAP directly. However, here's the tricky part: The reason this results in a SIGSEGV in Sage is because of Cysignals. Specifically, the custom "vectored continue handler" I added in cysignals#83 in order to better handle exceptions that occur on a sigaltstack. The reason I used a "vector continue handler" here is that, according to this invaluable analysis, a vectored continue handler can be run even when the kernel detects a failed SEH validation, which is what happens when you're suddenly running on a stack that Windows thinks is off in the weeds, as is the case with sigaltstack.

The problem is that vectored continue handlers also run after a normal exception occurs and the SEH handler returns ExceptionContinueExecution. This is what happens immediately after Cygwin's exception handler handles those explicit memory commits. So rather than returning from this particular exception normally, we wind up in Cysignals' win32_altstack_handler even though we should no longer be handling an exception, much less on an alt-stack.

One aspect of this that is not totally clear to me is why this doesn't happen normally: That is, why is this behavior normally avoided in cysignals' signal handling? If I had to wager a guess, it's simply that when cysignals does a longjmp from within the signal handler, we never return to the original exception handler in Cygwin and the vectored continue handler isn't run. Whereas, in this one obscure case, we wind up in the win32_altstack_handler before Cygwin even has a chance to send the process the SIGSEGV signal.

I need to see if there's way to modify win32_altstack_handler in order to detect this situation and avoid doing the wrong thing. For example, it should check whether or not we're actually running on the signal stack.

[embray]

comment:13

One quite easy workaround: Just bail early out of win32_altstack_handler completely if cysigs.inside_signal_handler. There's no reason to do anything in that function if we're not handling a signal in the first place.

This could still result in the undesired behavior if we happen to access some uninitialized memory in a MAP_NORESERVE mmap from within a signal handler. This seems generally unlikely to me, but perhaps a more reliable solution should be found for that case as well. But as a starting point this solves the basic problem completely.

[embray]

Dependencies: #27384

[embray]

Commit: f16f810

[embray]

Author: Erik Bray

[embray]

Branch: u/embray/cygwin/patch-gap-mmap-noreserve-pool

[embray]

comment:15

This adds a patch to GAP, which works fine on plain GAP but for use in Sage also requires #27384. By design the patch only affects GAP on Cygwin. In principle the same change could be made on other systems that support mmap() with the MAP_NORESERVE flag, though it's not clear whether or not that's strictly desirable.

It just means we can allocate a virtual address range for GAP's objects to live in up to any arbitrary size supported by the system, without regard to whether there is enough physical memory to use that entire address space (either at the time of allocation or sometime in the future) so it's still possible to run out of memory and get memory errors.

The reason this is needed on Cygwin is, as explained above, due to strange notions in Windows as to how copy-on-write pages are supposed to work, particularly in the context of making a MAP_PRIVATE mmap available to a child process after forking, which means that unfortunately such mmaps must be copied into the child process. Thus if a huge mmap needs to be made for GAP's memory pool, and then we fork the process, that entire mmap (even if much of it has not been written to yet) needs to copied in physical RAM. The only way to prevent this and keep the footprint of large mmaps small is to use the non-POSIX MAP_NORESERVE flag.

---

New commits:

f95dcff	Trac #27384: Patch cysignals with fix for Cygwin's sigaltstack exception
f16f810	Trac #27214: Patch GAP to allocate its memory pool using MAP_NORESERVE

[embray]

Upstream: Not yet reported upstream; Will do shortly.

[jdemeyer]

Changed dependencies from #27384 to #27070

[jdemeyer]

comment:17

1. Wouldn't it make sense to use MAP_NORESERVE unconditionally (on all systems)?

2. libgap memory allocation on Cygwin #27214 needs to be removed from this branch.

[embray]

Changed upstream from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.

[embray]

comment:25

After looking into this a bit more and obtaining some more stack traces, plus taking a look again at the multiprocessing.pool sources I see what's going on here:

In my example code in comment:23 the strange thing was that on the first p.apply(foo) it works, and then on the second one it segfaults. The reason for that is simply that Pool.__init__ directly calls Pool._repopulate_pool(), the method which forks off worker processes.

Thus, the N processes created at the time of Pool.__init__ are all forked directly from the main thread and function normally (in particular with the fix from #27384 in effect).

However, Pool then starts up a thread, as I mentioned previously, which is responsible for checking for completed worker processes and starting up new ones as needed. This is when things go awry. Cygwin supports forking from a thread just fine--perhaps too well in fact, because it successfully duplicates the thread and resumes execution on it in the new process, and this means it's susceptible to the bug I previously mentioned, where Cygwin exception handling wasn't working properly on threads, and in particular meaning that the first time accessing a mmap created with MAP_NORESERVE results immediately in an unhandled STATUS_ACCESS_VIOLATION, which Cysignals' win32_altstack_handler takes and exits the process with exit(-SIGSEGV) as though an unhandled SIGSEGV occurred.

I'm correct, then Cygwin 3.0.0 should fix that. I'm trying that next.

In the meantime a workaround is still needed. I would propose re-introducing the option to build the docs in serial, without using multiprocessing at all. This could be hidden normally much like the doctest runner's --serial flag, and on Cygwin use that at least for Cygwin<3.0.0.

Another option we've talked about before is replacing multiprocessing.Pool with something else which doesn't require threads, such as a generalization of sage.doctest.forker. I would still like to do that. But I think in the short-term a simpler hack is needed. After all, most other problems with using multiprocessing here have been otherwise dealt with, it's just this last stupid issue with Cygwin+gap+mmap+fork :(

[embray]

comment:26

Yep, Cygwin 3.0 fixes it. So that's good news at least. I'll make sure Cygwin>=3.0 is used for new builds of Sage and for the buildbot (probably >=3.0.4 since 3.0.0 had some unrelated problems with fork()). It will still be good to have a workaround though.

[embray]

comment:27

Okay, I'm going to rebase this to work with #27070, and I'm adding a little patch I'm experimenting with currently to work around the docbuild issue on older Cygwins that have this bug.

[embray]

Changed dependencies from #27070 to #27070, #27490

[vbraun]

comment:29

I think I mentioned the MAP_NORESERVE once to gap developers and they were sympathetic; IMHO it should definitely be upstreamed.

Realistically, this ticket is not going to make it for Sage 8.7 though.

[embray]

comment:31

Replying to @vbraun:

I think I mentioned the MAP_NORESERVE once to gap developers and they were sympathetic; IMHO it should definitely be upstreamed.

Realistically, this ticket is not going to make it for Sage 8.7 though.

What does "realistically" mean in this case? On what basis? I gave you plenty of advance notice that it's needed.

[jdemeyer]

comment:32

Did you forget to update the branch to #27070?

[sagetrac-git]

Changed commit from f16f810 to 4b359de

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

14b1551	Upgrade to Cysignals 1.10.2
a2ac9c7	cysignals should be a normal dependency
4b359de	Trac #27214: Patch GAP to allocate its memory pool using MAP_NORESERVE

[embray]

comment:34

I thought I did, but apparently not? I must have pushed to a different remote.

[jdemeyer]

comment:35

Please add a reference to the upstream PR in the .patch file. Once you did that, you can set this to positive review.

[jdemeyer]

Reviewer: Jeroen Demeyer

[vbraun]

Changed branch from u/embray/cygwin/patch-gap-mmap-noreserve-pool to 4b359de