Re: [PATCH v2] xmmap: inform Linux users of tuning knobs on ENOMEM
- From
Jeff King <peff@peff.net>
- Date
- Jun 30, 2021, 17:18 UTC
- Message-ID
- <YNynXZkDOXJDeR4P@coredump.intra.peff.net>
- In-Reply-To
- <20210630100722.GB2653@dcvr>
On Wed, Jun 30, 2021 at 10:07:22AM +0000, Eric Wong wrote:
Show 11 quoted lines
> > In general I agree that trying to manage our map count can never be > > foolproof. As you note, other parts of the system may contribute to that > > count. But even within Git, we don't have any mechanism for unmapping > > many non-packfiles. So if you have 30,000 packs, you may hit the limit > > purely via the .idx files (and ditto for the new .rev files, and > > probably commit-graph files, etc). > > Yeah, the most annoying thing with my original series was when > I hit "inflate: out of memory" once I stopped xmmap from dying. > I suspect that would be a worse far error message for users who > aren't familiar with how malloc works.
Yep. When I've seen this problem in the wild, that was exactly the message I hit, too.
Show 9 quoted lines
> > That said, I'm not opposed to handling xmmap() failures more gracefully, > > as your series did. It's not foolproof, but it might help in some cases. > > I've also been wondering if we can maintain a watermark based > on reading the contents of /proc/sys/vm/max_map_count and the > mappings we make. Then we could start dropping mappings > when we hit half (or some other threshold) of that sysctl. > Similar for RLIMIT_DATA (though that defaults to unlimited > on my Debian system).
Maybe. You'd still need to teach Git to start closing pack idx files.
One downside of closing any maps is that it leads to races with simultaneous repacks. Usually if a pack goes away we'll re-scan the objects directory and look for new packs. But in pack-objects, we commit ourselves to a certain packed representation. If that pack goes away (and we aren't holding on to it via an mmap or open descriptor), then we have to bail.
If we hit a point where the alternative is calling die() because we couldn't map or allocate something, then risking the race is OK. But unmapping at a more conservative level opens us up to that race even when we would have otherwise succeeded.
Show 6 quoted lines
> OTOH, I also wonder if we're overusing mmap when we could be > just as well served with pread. > > I'm not up-to-date on modern mmap performance and maybe CPU > vulnerability mitigations nowadays make mmap more compelling. > However, once upon a time in 2006, pread could be a hair quicker:
You can compile with NO_MMAP now to emulate it with pread(), but I doubt it performs that well. If you shrink the packedGitWindowSize, then I think use_pack() would bring in and operate on reasonable amounts of bytes at a time.
But lots of other code will mmap whole files (.idx code, packed-refs lookup, the index). We'd end up copying those files entirely into memory, rather than accessing them directly page-wise. That increases RAM usage (we are copying into our own heap) and makes operations inherently O(n) in the file sizes (one of the big reasons we mmap the packed-refs file is so that we can binary search without having to process the whole thing).
Those cases could be converted to use pread() more directly. Doing so for .idx files might not be too bad, as the records are fixed-length and we know where each step of the binary search will land. For packed-refs it would get pretty hairy; we jump to a particular offset and then have to scan around (sometimes backwards) for the beginning of the record.
Show 8 quoted lines
> > > So I think giving users some information to feed their sysadmins > > > is the best we can do in this situation: > > > > This seems OK to me, too. Translators might complain a bit about the > > message-lego. I don't have a strong opinion. > > *shrug* I saw my original patches already ended up in `seen' > (commit 7b79212a93c375365c06cab5c0018ab97a4185cf)
That doesn't necessarily mean much. Only that the topic was "seen" by the maintainer and didn't look like complete garbage. :)
-Peff