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? > After some consideration I think they do. Grep threads definitly get the try_to_free function registered while the main one does at least some xreallock out of sha1_file.c (so not owning the lock). I guess it requires the same kind of lock pack-objects.c uses (meaning we need to set the try_to_free function in grep.c too). > > [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. I'm not expert enough in git pathways to be sure about pack_objects but I agree it looks "risky". Maybe add_packed_git should check whether there already is a free routine (other than do_nothing) instead of simply setting it up the first time. 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