On 08/31/2011 03:59 AM, Jeff King wrote: > I notice there are some other code paths that end up in xmalloc without > locking, too (e.g., load_file, and some strbuf_* calls). Don't those > need locking, too, as malloc may try to release packfile memory? > > builtin/pack-objects.c dealt with this already by setting a new > "try_to_free" routine that locks[1], which we should also do. It > probably comes up less frequently, because it only happens when we're > under memory pressure. > > -Peff > > [1] Actually, it looks like the "try_to_free" routine starts as nothing, > and then add_packed_git sets it lazily to try_to_free_pack_memory. > But what builtin/pack-objects tries to do is overwrite that with a > version of try_to_free_pack_memory that does locking. So it's > possible that we would not have read any packed objects while > setting up the threads, and add_packed_git will overwrite our > careful, locking version of try_to_free_pack_memory. > > I _think_ pack-objects is probably OK, because it will have already > done the complete "counting objects" phase, which would look in any > packs. But it may be harder for grep. > After some more looking around, I'd say the best way to fix this is provide a lock to the try_to_free function from sha1_file and provide access to this lock for pack-objects and grep to replace respectively the read_mutex and read_sha1_mutex. So we can simplify the problem by having a single lock to avoid all the cache/free issues (and reusable elsewhere if needed in the future), whether it's shared through direct access or API (I'm not sure what's git policy about that). And this way, there is no need to duplicate what pack-objects is achieving and it gives some peace of mind about the fact that the try_to _free function won't be overwritten in our backs. Nicolas Morey-Chaisemartin -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html