Re: [PATCH] khash: clarify that allocations never fail
- From
Jeff King <peff@peff.net>
- Date
- Jul 4, 2021, 10:11 UTC
- Message-ID
- <YOGJOO6l6bIsISkT@coredump.intra.peff.net>
- In-Reply-To
- <b2c9b072-b2ac-625b-4f0f-9ba1ca3321d7@web.de>
On Sun, Jul 04, 2021 at 11:41:23AM +0200, René Scharfe wrote:
Show 9 quoted lines
> Btw. the found code is:
>
> start = xmalloc(length);
> if (start == NULL) {
> errno = ENOMEM;
> return MAP_FAILED;
> }
>
> start cannot be NULL, so the check is dead code.Yep, so it's doubly silly.
Show 8 quoted lines
> > IMHO that should not be using xmalloc() at all. It is a replacement for > > system mmap, which can fail with ENOMEM, and we should be able to do the > > same. Using xmalloc here is probably losing an opportunity to close > > another pack window to free up memory for a new one. > > Do you mean using malloc(3) directly instead of xmalloc() would no > longer try to release pack windows? xmalloc() hasn't done that anymore > since 9827d4c185 (packfile: drop release_pack_memory(), 2019-08-12).
No, I meant that by using xmalloc() here, if the allocation fails, we'll immediately die(). Whereas the caller of mmap() could get the ENOMEM error, then unmap an in-use pack window and try again.
However, I forgot that we don't actually do that (yet). We unmap windows if we go over our own packed_git_window_size counter, but not when mmap fails. Eric posted a patch recently to change that, though (at which point the die() in xmalloc() would be working against us).
(Actually, we did _try_ to do something like that in xmmap prior to 9827d4c185, but I don't think it actually worked since it was based on our own internal limit, and not what the OS would allow).
Show 5 quoted lines
> xmalloc() still brings support for zero-sized allocations on platforms > whose malloc(3) doesn't like them, and it enforces GIT_ALLOC_LIMIT. > mmap() is supposed give up with EINVAL if the length is 0, so the > first feature is not actually helping. And GIT_ALLOC_LIMIT is not > documented and only useful for testing, right?
Yeah, I think failing on a 0-length mmap is OK, since that's what real systems do (and this is a wrapper). Our xmmap_gently() handles this, which is the right spot.
I don't think of GIT_ALLOC_LIMIT as something we're committed to publicly supporting, though I have (ab)used it once or twice myself. However, I'm not sure if it makes sense here. True, on a system without mmap it is a heap allocation, but to me it's conceptually very different than a normal allocation (and on a system with mmap, we have no problem at all requesting arbitrarily large maps). If we did want to put a limit here, I think we'd want to do it at the xmmap layer, using a separate variable.
-Peff