{"thread":{"id":"23152","subject":"[PATCH 1/2] Make xmalloc and xrealloc thread-safe","startedAt":"2010-03-23T17:31:14Z","lastAt":"2010-04-08T08:42:28Z","messageCount":39,"participants":["Fredrik Kuivinen","Shawn O. Pearce","Johannes Sixt","Nicolas Pitre","Shawn Pearce","Junio C Hamano","Sverre Rabbelier","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"137652","messageId":"20100323173114.GB4218@fredrik-laptop","threadId":"23152","inReplyTo":"20100323161713.3183.57927.stgit@fredrik-laptop","subject":"[PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-23T17:31:14Z","receivedAt":"2010-03-23T17:31:14Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"\nSigned-off-by: Fredrik Kuivinen \n---\n\n builtin/grep.c | 2 +-\n builtin/pack-objects.c | 4 ++--\n git-compat-util.h | 8 ++++++++\n preload-index.c | 2 +-\n run-command.c | 3 ++-\n wrapper.c | 22 ++++++++++++++++------\n 6 files changed, 30 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9d30ddb..78b0bf4 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -236,7 +236,7 @@ static void start_threads(struct grep_opt *opt)\n \t\tstruct grep_opt *o = grep_opt_dup(opt);\n \t\to->output = strbuf_out;\n \t\tcompile_grep_patterns(o);\n-\t\terr = pthread_create(&threads[i], NULL, run, o);\n+\t\terr = xpthread_create(&threads[i], NULL, run, o);\n \n \t\tif (err)\n \t\t\tdie(\"grep: failed to create thread: %s\",\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 9780258..022b6a8 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1651,8 +1651,8 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n \t\t\tcontinue;\n \t\tpthread_mutex_init(&p[i].mutex, NULL);\n \t\tpthread_cond_init(&p[i].cond, NULL);\n-\t\tret = pthread_create(&p[i].thread, NULL,\n-\t\t\t\t threaded_find_deltas, &p[i]);\n+\t\tret = xpthread_create(&p[i].thread, NULL,\n+\t\t\t\t threaded_find_deltas, &p[i]);\n \t\tif (ret)\n \t\t\tdie(\"unable to create thread: %s\", strerror(ret));\n \t\tactive_threads++;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex aebd9cd..fe10901 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -140,6 +140,10 @@ extern char *gitbasename(char *);\n #include \n #endif\n \n+#ifndef NO_PTHREADS\n+#include \n+#endif\n+\n /* On most systems would have given us this, but\n * not on some systems (e.g. GNU/Hurd).\n */\n@@ -356,6 +360,10 @@ static inline void *gitmempcpy(void *dest, const void *src, size_t n)\n \n extern void release_pack_memory(size_t, int);\n \n+#ifndef NO_PTHREADS\n+extern int xpthread_create(pthread_t *thread, const pthread_attr_t *attr,\n+\t\t\t void *(*start_routine)(void*), void *arg);\n+#endif\n extern char *xstrdup(const char *str);\n extern void *xmalloc(size_t size);\n extern void *xmallocz(size_t size);\ndiff --git a/preload-index.c b/preload-index.c\nindex e3d0bda..250bb0b 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -86,7 +86,7 @@ static void preload_index(struct index_state *index, const char **pathspec)\n \t\tp->offset = offset;\n \t\tp->nr = work;\n \t\toffset += work;\n-\t\tif (pthread_create(&p->pthread, NULL, preload_thread, p))\n+\t\tif (xpthread_create(&p->pthread, NULL, preload_thread, p))\n \t\t\tdie(\"unable to create threaded lstat\");\n \t}\n \tfor (i = 0; i < threads; i++) {\ndiff --git a/run-command.c b/run-command.c\nindex e996b21..1e4def4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -568,7 +568,8 @@ int start_async(struct async *async)\n \tasync->proc_in = proc_in;\n \tasync->proc_out = proc_out;\n \t{\n-\t\tint err = pthread_create(&async->tid, NULL, run_thread, async);\n+\t\tint err = xpthread_create(&async->tid, NULL, run_thread,\n+\t\t\t\t\t async);\n \t\tif (err) {\n \t\t\terror(\"cannot create thread: %s\", strerror(err));\n \t\t\tgoto error;\ndiff --git a/wrapper.c b/wrapper.c\nindex 9c71b21..e7140d1 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -15,19 +15,29 @@ char *xstrdup(const char *str)\n \treturn ret;\n }\n \n+static int multiple_threads;\n+#ifndef NO_PTHREADS\n+int xpthread_create(pthread_t *thread, const pthread_attr_t *attr,\n+\t\t void *(*start_routine)(void*), void *arg)\n+{\n+\tmultiple_threads = 1;\n+\treturn pthread_create(thread, attr, start_routine, arg);\n+}\n+#endif\n+\n void *xmalloc(size_t size)\n {\n \tvoid *ret = malloc(size);\n \tif (!ret && !size)\n \t\tret = malloc(1);\n-\tif (!ret) {\n+\tif (!ret && !multiple_threads) {\n \t\trelease_pack_memory(size, -1);\n \t\tret = malloc(size);\n \t\tif (!ret && !size)\n \t\t\tret = malloc(1);\n-\t\tif (!ret)\n-\t\t\tdie(\"Out of memory, malloc failed\");\n \t}\n+\tif (!ret)\n+\t\tdie(\"Out of memory, malloc failed\");\n #ifdef XMALLOC_POISON\n \tmemset(ret, 0xA5, size);\n #endif\n@@ -66,14 +76,14 @@ void *xrealloc(void *ptr, size_t size)\n \tvoid *ret = realloc(ptr, size);\n \tif (!ret && !size)\n \t\tret = realloc(ptr, 1);\n-\tif (!ret) {\n+\tif (!ret && !multiple_threads) {\n \t\trelease_pack_memory(size, -1);\n \t\tret = realloc(ptr, size);\n \t\tif (!ret && !size)\n \t\t\tret = realloc(ptr, 1);\n-\t\tif (!ret)\n-\t\t\tdie(\"Out of memory, realloc failed\");\n \t}\n+\tif (!ret)\n+\t\tdie(\"Out of memory, realloc failed\");\n \treturn ret;\n }\n \n"},{"id":"137653","messageId":"20100323173130.GC4218@fredrik-laptop","threadId":"23152","inReplyTo":"20100323161713.3183.57927.stgit@fredrik-laptop","subject":"[PATCH 2/2] Make sha1_to_hex thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-23T17:31:30Z","receivedAt":"2010-03-23T17:31:30Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"\nSigned-off-by: Fredrik Kuivinen \n---\n\n hex.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 53 insertions(+), 0 deletions(-)\n\ndiff --git a/hex.c b/hex.c\nindex bb402fb..fe1d302 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -1,5 +1,9 @@\n #include \"cache.h\"\n \n+#ifndef NO_PTHREADS\n+#include \n+#endif\n+\n const signed char hexval_table[256] = {\n \t -1, -1, -1, -1, -1, -1, -1, -1,\t\t/* 00-07 */\n \t -1, -1, -1, -1, -1, -1, -1, -1,\t\t/* 08-0f */\n@@ -48,6 +52,7 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)\n \treturn 0;\n }\n \n+#ifdef NO_PTHREADS\n char *sha1_to_hex(const unsigned char *sha1)\n {\n \tstatic int bufno;\n@@ -65,3 +70,51 @@ char *sha1_to_hex(const unsigned char *sha1)\n \n \treturn buffer;\n }\n+#else\n+static pthread_once_t sha1_to_hex_once = PTHREAD_ONCE_INIT;\n+static pthread_key_t sha1_to_hex_key;\n+\n+static void sha1_to_hex_init(void)\n+{\n+\tint err = pthread_key_create(&sha1_to_hex_key, free);\n+\tif (err)\n+\t\tdie(\"pthread_key_create failed: %s\", strerror(err));\n+}\n+\n+struct sha1_to_hex_buf\n+{\n+\tint no;\n+\tchar hex[4][50];\n+};\n+\n+char *sha1_to_hex(const unsigned char *sha1)\n+{\n+\tstatic const char hex[] = \"0123456789abcdef\";\n+\tstruct sha1_to_hex_buf *hexbuf;\n+\tint i;\n+\tchar *buffer, *buf;\n+\n+\tpthread_once(&sha1_to_hex_once, sha1_to_hex_init);\n+\n+\thexbuf = pthread_getspecific(sha1_to_hex_key);\n+\tif (!hexbuf) {\n+\t\tint err;\n+\t\thexbuf = xmalloc(sizeof(struct sha1_to_hex_buf));\n+\t\thexbuf->no = 0;\n+\t\terr = pthread_setspecific(sha1_to_hex_key, hexbuf);\n+\t\tif (err)\n+\t\t\tdie(\"pthread_getspecific failed: %s\", strerror(err));\n+\t}\n+\n+\tbuffer = hexbuf->hex[3 & ++hexbuf->no], buf = buffer;\n+\n+\tfor (i = 0; i < 20; i++) {\n+\t\tunsigned int val = *sha1++;\n+\t\t*buf++ = hex[val >> 4];\n+\t\t*buf++ = hex[val & 0xf];\n+\t}\n+\t*buf = '\\0';\n+\n+\treturn buffer;\n+}\n+#endif\n"},{"id":"137660","messageId":"20100323184309.GA31668@spearce.org","threadId":"23152","inReplyTo":"20100323173114.GB4218@fredrik-laptop","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-23T18:43:09Z","receivedAt":"2010-03-23T18:43:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Fredrik Kuivinen wrote:\n> +static int multiple_threads;\n> +#ifndef NO_PTHREADS\n> +int xpthread_create(pthread_t *thread, const pthread_attr_t *attr,\n> +\t\t void *(*start_routine)(void*), void *arg)\n> +{\n> +\tmultiple_threads = 1;\n> +\treturn pthread_create(thread, attr, start_routine, arg);\n> +}\n> +#endif\n> +\n> void *xmalloc(size_t size)\n> {\n> \tvoid *ret = malloc(size);\n> \tif (!ret && !size)\n> \t\tret = malloc(1);\n> -\tif (!ret) {\n> +\tif (!ret && !multiple_threads) {\n> \t\trelease_pack_memory(size, -1);\n\nSo by \"make thread safe\" you really mean \"disable release of\nleast-frequently used pack windows once any thread starts\".\n\nIf that is what we are doing, disabling the release of pack windows\nwhen malloc fails, why can't we do that all of the time?\n\n-- \nShawn.\n"},{"id":"137666","messageId":"201003232123.59407.j6t@kdbg.org","threadId":"23152","inReplyTo":"20100323173130.GC4218@fredrik-laptop","subject":"Re: [PATCH 2/2] Make sha1_to_hex thread-safe","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-23T20:23:59Z","receivedAt":"2010-03-23T20:23:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"As I said in my previous mail, this is not necessary.\n\nIn particular...\n\nOn Dienstag, 23. März 2010, Fredrik Kuivinen wrote:\n> +static pthread_once_t sha1_to_hex_once = PTHREAD_ONCE_INIT;\n\n... please give us a break: Don't force us to pile missing pthreads features \nin our Windows layer.\n\n-- Hannes\n"},{"id":"137670","messageId":"4c8ef71003231421u789c4332h461c066add0ec7b1@mail.gmail.com","threadId":"23152","inReplyTo":"20100323184309.GA31668@spearce.org","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-23T21:21:40Z","receivedAt":"2010-03-23T21:21:40Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Tue, Mar 23, 2010 at 19:43, Shawn O. Pearce wrote:\n> Fredrik Kuivinen wrote:\n>> +static int multiple_threads;\n>> +#ifndef NO_PTHREADS\n>> +int xpthread_create(pthread_t *thread, const pthread_attr_t *attr,\n>> +                 void *(*start_routine)(void*), void *arg)\n>> +{\n>> +     multiple_threads = 1;\n>> +     return pthread_create(thread, attr, start_routine, arg);\n>> +}\n>> +#endif\n>> +\n>>  void *xmalloc(size_t size)\n>>  {\n>>       void *ret = malloc(size);\n>>       if (!ret && !size)\n>>               ret = malloc(1);\n>> -     if (!ret) {\n>> +     if (!ret && !multiple_threads) {\n>>               release_pack_memory(size, -1);\n>\n> So by \"make thread safe\" you really mean \"disable release of\n> least-frequently used pack windows once any thread starts\".\n\nYes.\n\n> If that is what we are doing, disabling the release of pack windows\n> when malloc fails, why can't we do that all of the time?\n\nThe idea was that most git programs are single threaded, so they can\nstill benefit from releasing the pack windows when they are low on\nmemory.\n\n- Fredrik\n"},{"id":"137677","messageId":"alpine.LFD.2.00.1003231945480.31128@xanadu.home","threadId":"23152","inReplyTo":"4c8ef71003231421u789c4332h461c066add0ec7b1@mail.gmail.com","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-03-23T23:50:19Z","receivedAt":"2010-03-23T23:50:19Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 23 Mar 2010, Fredrik Kuivinen wrote:\n\n> On Tue, Mar 23, 2010 at 19:43, Shawn O. Pearce wrote:\n> > If that is what we are doing, disabling the release of pack windows\n> > when malloc fails, why can't we do that all of the time?\n> \n> The idea was that most git programs are single threaded, so they can\n> still benefit from releasing the pack windows when they are low on\n> memory.\n\nThis is bobus. The Git program using the most memory is probably \npack-objects and it is threaded. Most single-threaded programs don't \nuse close to as much memory.\n\n\nNicolas\n"},{"id":"137704","messageId":"4c8ef71003240823o7cd733bn5f19699305c94cba@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1003231945480.31128@xanadu.home","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-24T15:23:29Z","receivedAt":"2010-03-24T15:23:29Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Mar 24, 2010 at 00:50, Nicolas Pitre wrote:\n> On Tue, 23 Mar 2010, Fredrik Kuivinen wrote:\n>\n>> On Tue, Mar 23, 2010 at 19:43, Shawn O. Pearce wrote:\n>> > If that is what we are doing, disabling the release of pack windows\n>> > when malloc fails, why can't we do that all of the time?\n>>\n>> The idea was that most git programs are single threaded, so they can\n>> still benefit from releasing the pack windows when they are low on\n>> memory.\n>\n> This is bobus. The Git program using the most memory is probably\n> pack-objects and it is threaded.  Most single-threaded programs don't\n> use close to as much memory.\n\nOk, you are right. But xmalloc/xrealloc cannot be used in multiple\nthreads simultaneously without some serialization.\n\nFor example, I think there are some potential race conditions in the\npack-objects code. In the threaded code we have the following call\nchains leading to xcalloc, xmalloc, and xrealloc:\n\nfind_deltas -> xcalloc\nfind_deltas -> do_compress -> xmalloc\nfind_deltas -> try_delta -> xrealloc\nfind_deltas -> try_delta -> read_sha1_file -> ... -> xmalloc (called\nwith read_lock held, but it can still race with the other calls)\n\nAs far as I can see there is no serialization between these calls.\n\n- Fredrik\n"},{"id":"137716","messageId":"alpine.LFD.2.00.1003241133430.694@xanadu.home","threadId":"23152","inReplyTo":"4c8ef71003240823o7cd733bn5f19699305c94cba@mail.gmail.com","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-03-24T17:53:01Z","receivedAt":"2010-03-24T17:53:01Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 24 Mar 2010, Fredrik Kuivinen wrote:\n\n> On Wed, Mar 24, 2010 at 00:50, Nicolas Pitre wrote:\n> > On Tue, 23 Mar 2010, Fredrik Kuivinen wrote:\n> >\n> >> On Tue, Mar 23, 2010 at 19:43, Shawn O. Pearce wrote:\n> >> > If that is what we are doing, disabling the release of pack windows\n> >> > when malloc fails, why can't we do that all of the time?\n> >>\n> >> The idea was that most git programs are single threaded, so they can\n> >> still benefit from releasing the pack windows when they are low on\n> >> memory.\n> >\n> > This is bobus. The Git program using the most memory is probably\n> > pack-objects and it is threaded.  Most single-threaded programs don't\n> > use close to as much memory.\n> \n> Ok, you are right. But xmalloc/xrealloc cannot be used in multiple\n> threads simultaneously without some serialization.\n> \n> For example, I think there are some potential race conditions in the\n> pack-objects code. In the threaded code we have the following call\n> chains leading to xcalloc, xmalloc, and xrealloc:\n> \n> find_deltas -> xcalloc\n> find_deltas -> do_compress -> xmalloc\n> find_deltas -> try_delta -> xrealloc\n> find_deltas -> try_delta -> read_sha1_file -> ... -> xmalloc (called\n> with read_lock held, but it can still race with the other calls)\n> \n> As far as I can see there is no serialization between these calls.\n\nTrue. We already have a problem. This is nasty.\n\n\nNicolas\n"},{"id":"137717","messageId":"ec874dac1003241122s3d592f26n1b23d23144939218@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1003241133430.694@xanadu.home","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-24T18:22:23Z","receivedAt":"2010-03-24T18:22:23Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Mar 24, 2010 at 10:53 AM, Nicolas Pitre wrote:\n> On Wed, 24 Mar 2010, Fredrik Kuivinen wrote:\n>\n>> On Wed, Mar 24, 2010 at 00:50, Nicolas Pitre wrote:\n>> > On Tue, 23 Mar 2010, Fredrik Kuivinen wrote:\n>> >\n>> >> On Tue, Mar 23, 2010 at 19:43, Shawn O. Pearce wrote:\n>> >> > If that is what we are doing, disabling the release of pack windows\n>> >> > when malloc fails, why can't we do that all of the time?\n>> >>\n>> >> The idea was that most git programs are single threaded, so they can\n>> >> still benefit from releasing the pack windows when they are low on\n>> >> memory.\n>> >\n>> > This is bobus. The Git program using the most memory is probably\n>> > pack-objects and it is threaded.  Most single-threaded programs don't\n>> > use close to as much memory.\n>>\n>> Ok, you are right. But xmalloc/xrealloc cannot be used in multiple\n>> threads simultaneously without some serialization.\n>>\n>> For example, I think there are some potential race conditions in the\n>> pack-objects code. In the threaded code we have the following call\n>> chains leading to xcalloc, xmalloc, and xrealloc:\n>>\n>> find_deltas -> xcalloc\n>> find_deltas -> do_compress -> xmalloc\n>> find_deltas -> try_delta -> xrealloc\n>> find_deltas -> try_delta -> read_sha1_file -> ... -> xmalloc  (called\n>> with read_lock held, but it can still race with the other calls)\n>>\n>> As far as I can see there is no serialization between these calls.\n>\n> True.  We already have a problem.  This is nasty.\n\nThe easy solution is probably to remove the use of xmalloc from\nfind_deltas code path. But then we run into hard failures when we\ncan't get the memory we need, there isn't a way to recover from a\nmalloc() failure deep within read_sha1_file for example. The current\nsolution is the best we can do, try to ditch pack windows and hope\nthat releases sufficient virtual memory space that a second malloc()\nattempt can succeed by increasing heap.\n\nWe could use a mutex during the malloc failure code-path of xmalloc,\nto ensure only one thread goes through that pack window cleanup at a\ntime. But that will still mess with the main thread which doesn't\nreally want to acquire mutexes during object access as it uses the\nexisting pack windows.\n\nI thought pack-objects did all object access from the main thread and\nonly delta searches on the worker threads? If that is true, maybe we\ncan have the worker threads signal the main thread on malloc failure\nto release pack windows, and then wait for that signal to be\nacknowledged before they attempt to retry the malloc. This means the\nmain thread would need to periodically test that condition as its\ndispatching batches of objects to the workers.\n\nUgly.\n\n-- \nShawn.\n"},{"id":"137719","messageId":"7vaatxk19e.fsf@alter.siamese.dyndns.org","threadId":"23152","inReplyTo":"ec874dac1003241122s3d592f26n1b23d23144939218@mail.gmail.com","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-24T18:44:45Z","receivedAt":"2010-03-24T18:44:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> I thought pack-objects did all object access from the main thread and\n> only delta searches on the worker threads?\n\nHmm, you lost me. try_delta() is the one that reads the data out of\neither loose object or from an existing pack for comparison lazily, and\nthat is what each worker thread runs repeatedly in find_deltas()...\n"},{"id":"137722","messageId":"alpine.LFD.2.00.1003241435300.694@xanadu.home","threadId":"23152","inReplyTo":"ec874dac1003241122s3d592f26n1b23d23144939218@mail.gmail.com","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-03-24T18:54:48Z","receivedAt":"2010-03-24T18:54:48Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 24 Mar 2010, Shawn Pearce wrote:\n\n> On Wed, Mar 24, 2010 at 10:53 AM, Nicolas Pitre wrote:\n> > On Wed, 24 Mar 2010, Fredrik Kuivinen wrote:\n> >\n> >> On Wed, Mar 24, 2010 at 00:50, Nicolas Pitre wrote:\n> >> > On Tue, 23 Mar 2010, Fredrik Kuivinen wrote:\n> >> >\n> >> >> On Tue, Mar 23, 2010 at 19:43, Shawn O. Pearce wrote:\n> >> >> > If that is what we are doing, disabling the release of pack windows\n> >> >> > when malloc fails, why can't we do that all of the time?\n> >> >>\n> >> >> The idea was that most git programs are single threaded, so they can\n> >> >> still benefit from releasing the pack windows when they are low on\n> >> >> memory.\n> >> >\n> >> > This is bobus. The Git program using the most memory is probably\n> >> > pack-objects and it is threaded.  Most single-threaded programs don't\n> >> > use close to as much memory.\n> >>\n> >> Ok, you are right. But xmalloc/xrealloc cannot be used in multiple\n> >> threads simultaneously without some serialization.\n> >>\n> >> For example, I think there are some potential race conditions in the\n> >> pack-objects code. In the threaded code we have the following call\n> >> chains leading to xcalloc, xmalloc, and xrealloc:\n> >>\n> >> find_deltas -> xcalloc\n> >> find_deltas -> do_compress -> xmalloc\n> >> find_deltas -> try_delta -> xrealloc\n> >> find_deltas -> try_delta -> read_sha1_file -> ... -> xmalloc  (called\n> >> with read_lock held, but it can still race with the other calls)\n> >>\n> >> As far as I can see there is no serialization between these calls.\n> >\n> > True.  We already have a problem.  This is nasty.\n> \n> The easy solution is probably to remove the use of xmalloc from\n> find_deltas code path. But then we run into hard failures when we\n> can't get the memory we need, there isn't a way to recover from a\n> malloc() failure deep within read_sha1_file for example.\n\nThe read_sha1_file path is not the problem -- it is always protected \nagainst concurrency with a mutex.\n\nIt is more about do_compress() called on line 1476 of pack-objects.c for \nexample.\n\n\n> The current solution is the best we can do, try to ditch pack windows \n> and hope that releases sufficient virtual memory space that a second \n> malloc() attempt can succeed by increasing heap.\n> \n> We could use a mutex during the malloc failure code-path of xmalloc,\n> to ensure only one thread goes through that pack window cleanup at a\n> time. But that will still mess with the main thread which doesn't\n> really want to acquire mutexes during object access as it uses the\n> existing pack windows.\n\nRight.\n\n> I thought pack-objects did all object access from the main thread and\n> only delta searches on the worker threads?\n\nNo. Each thread is responsible for grabbing its own data set.\n\n> If that is true, maybe we\n> can have the worker threads signal the main thread on malloc failure\n> to release pack windows, and then wait for that signal to be\n> acknowledged before they attempt to retry the malloc. This means the\n> main thread would need to periodically test that condition as its\n> dispatching batches of objects to the workers.\n> \n> Ugly.\n\nIndeed.\n\nThe real solution, of course, would be to have pack window manipulations \nprotected by a mutex of its own. This, plus another mutex for the delta \nbase cache, and then read_sha1_file() could almost be reentrant.\n\nAnother solution could be for xmalloc() to use a function pointer for \nthe method to use on malloc error path, which would default to a \nfunction calling release_pack_memory(size, -1). Then pack-objects.c \nwould override the default with its own to acquire the read_mutex around \nthe call to release_pack_memory(). That is probably the easiest \nsolution for now.\n\n\nNicolas\n"},{"id":"137728","messageId":"ec874dac1003241257r3cad86c9q1af84d3732e23ca8@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1003241435300.694@xanadu.home","subject":"Re: [PATCH 1/2] Make xmalloc and xrealloc thread-safe","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-24T19:57:38Z","receivedAt":"2010-03-24T19:57:38Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Mar 24, 2010 at 11:54 AM, Nicolas Pitre wrote:\n> Another solution could be for xmalloc() to use a function pointer for\n> the method to use on malloc error path, which would default to a\n> function calling release_pack_memory(size, -1).  Then pack-objects.c\n> would override the default with its own to acquire the read_mutex around\n> the call to release_pack_memory().  That is probably the easiest\n> solution for now.\n\nYea, that sounds like the most reasonable solution right now.\n\n-- \nShawn.\n"},{"id":"137734","messageId":"alpine.LFD.2.00.1003241613020.694@xanadu.home","threadId":"23152","inReplyTo":"ec874dac1003241257r3cad86c9q1af84d3732e23ca8@mail.gmail.com","subject":"[PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-03-24T20:22:34Z","receivedAt":"2010-03-24T20:22:34Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"By providing a hook for the routine responsible for trying to free some \nmemory on malloc failure, we can ensure that the called routine is\nprotected by the appropriate locks when threads are in play.\n\nThe obvious offender here was pack-objects which was calling xmalloc()\nwithin threads while release_pack_memory() is not thread safe.\n\nSigned-off-by: Nicolas Pitre \n---\n\nOn Wed, 24 Mar 2010, Shawn Pearce wrote:\n\n> On Wed, Mar 24, 2010 at 11:54 AM, Nicolas Pitre wrote:\n> > Another solution could be for xmalloc() to use a function pointer for\n> > the method to use on malloc error path, which would default to a\n> > function calling release_pack_memory(size, -1).  Then pack-objects.c\n> > would override the default with its own to acquire the read_mutex around\n> > the call to release_pack_memory().  That is probably the easiest\n> > solution for now.\n> \n> Yea, that sounds like the most reasonable solution right now.\n\nSo here it is.\n\nNote: there was a dubious usage of fd when calling release_pack_memory() \nin xmmap() which is now removed.\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 9780258..65f797f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1522,6 +1522,13 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n \n #ifndef NO_PTHREADS\n \n+static void try_to_free_from_threads(size_t size)\n+{\n+\tread_lock();\n+\trelease_pack_memory(size, -1);\n+\tread_unlock();\n+}\n+\n /*\n * The main thread waits on the condition that (at least) one of the workers\n * has stopped working (which is indicated in the .working member of\n@@ -1556,10 +1563,12 @@ static void init_threaded_search(void)\n \tpthread_mutex_init(&cache_mutex, NULL);\n \tpthread_mutex_init(&progress_mutex, NULL);\n \tpthread_cond_init(&progress_cond, NULL);\n+\tset_try_to_free_routine(try_to_free_from_threads);\n }\n \n static void cleanup_threaded_search(void)\n {\n+\tset_try_to_free_routine(NULL);\n \tpthread_cond_destroy(&progress_cond);\n \tpthread_mutex_destroy(&read_mutex);\n \tpthread_mutex_destroy(&cache_mutex);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex aebd9cd..53ab5aa 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -356,6 +356,8 @@ static inline void *gitmempcpy(void *dest, const void *src, size_t n)\n \n extern void release_pack_memory(size_t, int);\n \n+extern void set_try_to_free_routine(void (*routine)(size_t));\n+\n extern char *xstrdup(const char *str);\n extern void *xmalloc(size_t size);\n extern void *xmallocz(size_t size);\ndiff --git a/wrapper.c b/wrapper.c\nindex 9c71b21..8a4f3f2 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -3,11 +3,23 @@\n */\n #include \"cache.h\"\n \n+static void try_to_free_builtin(size_t size)\n+{\n+\trelease_pack_memory(size, -1);\n+}\n+\n+static void (*try_to_free_routine)(size_t size) = try_to_free_builtin;\n+\n+void set_try_to_free_routine(void (*routine)(size_t))\n+{\n+\ttry_to_free_routine = (routine) ? routine : try_to_free_builtin;\n+}\n+\n char *xstrdup(const char *str)\n {\n \tchar *ret = strdup(str);\n \tif (!ret) {\n-\t\trelease_pack_memory(strlen(str) + 1, -1);\n+\t\ttry_to_free_routine(strlen(str) + 1);\n \t\tret = strdup(str);\n \t\tif (!ret)\n \t\t\tdie(\"Out of memory, strdup failed\");\n@@ -21,7 +33,7 @@ void *xmalloc(size_t size)\n \tif (!ret && !size)\n \t\tret = malloc(1);\n \tif (!ret) {\n-\t\trelease_pack_memory(size, -1);\n+\t\ttry_to_free_routine(size);\n \t\tret = malloc(size);\n \t\tif (!ret && !size)\n \t\t\tret = malloc(1);\n@@ -67,7 +79,7 @@ void *xrealloc(void *ptr, size_t size)\n \tif (!ret && !size)\n \t\tret = realloc(ptr, 1);\n \tif (!ret) {\n-\t\trelease_pack_memory(size, -1);\n+\t\ttry_to_free_routine(size);\n \t\tret = realloc(ptr, size);\n \t\tif (!ret && !size)\n \t\t\tret = realloc(ptr, 1);\n@@ -83,7 +95,7 @@ void *xcalloc(size_t nmemb, size_t size)\n \tif (!ret && (!nmemb || !size))\n \t\tret = calloc(1, 1);\n \tif (!ret) {\n-\t\trelease_pack_memory(nmemb * size, -1);\n+\t\ttry_to_free_routine(nmemb * size);\n \t\tret = calloc(nmemb, size);\n \t\tif (!ret && (!nmemb || !size))\n \t\t\tret = calloc(1, 1);\n@@ -100,7 +112,7 @@ void *xmmap(void *start, size_t length,\n \tif (ret == MAP_FAILED) {\n \t\tif (!length)\n \t\t\treturn NULL;\n-\t\trelease_pack_memory(length, fd);\n+\t\ttry_to_free_routine(length);\n \t\tret = mmap(start, length, prot, flags, fd, offset);\n \t\tif (ret == MAP_FAILED)\n \t\t\tdie_errno(\"Out of memory? mmap failed\");\n"},{"id":"137737","messageId":"20100324202814.GA24830@spearce.org","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1003241613020.694@xanadu.home","subject":"Re: [PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-03-24T20:28:14Z","receivedAt":"2010-03-24T20:28:14Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> Note: there was a dubious usage of fd when calling release_pack_memory() \n> in xmmap() which is now removed.\n...\n> @@ -100,7 +112,7 @@ void *xmmap(void *start, size_t length,\n> \tif (ret == MAP_FAILED) {\n> \t\tif (!length)\n> \t\t\treturn NULL;\n> -\t\trelease_pack_memory(length, fd);\n> +\t\ttry_to_free_routine(length);\n\nThis isn't dubious! The fd passed here is to prevent the pack\nrelease code from closing this fd right before we try to mmap it.\nIts an actual bug fix that I had to write years ago, check the\nhistory of that section of code... :-)\n\n-- \nShawn.\n"},{"id":"137744","messageId":"alpine.LFD.2.00.1003241656010.694@xanadu.home","threadId":"23152","inReplyTo":"20100324202814.GA24830@spearce.org","subject":"Re: [PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-03-24T21:02:15Z","receivedAt":"2010-03-24T21:02:15Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 24 Mar 2010, Shawn O. Pearce wrote:\n\n> Nicolas Pitre wrote:\n> > Note: there was a dubious usage of fd when calling release_pack_memory() \n> > in xmmap() which is now removed.\n> ...\n> > @@ -100,7 +112,7 @@ void *xmmap(void *start, size_t length,\n> > \tif (ret == MAP_FAILED) {\n> > \t\tif (!length)\n> > \t\t\treturn NULL;\n> > -\t\trelease_pack_memory(length, fd);\n> > +\t\ttry_to_free_routine(length);\n> \n> This isn't dubious! The fd passed here is to prevent the pack\n> release code from closing this fd right before we try to mmap it.\n> Its an actual bug fix that I had to write years ago, check the\n> history of that section of code... :-)\n\nArgh. My bad. I somehow thought that fd was the actual pack to free \nwhen specified. Let's drop the very last hunk of the patch then. \nxmmap() is certainly not going to be invoked concurrently to the rest of \nsha1_file.c in a separate thread.\n\nJunio: I suppose you don't need me to resend?\n\n\nNicolas\n"},{"id":"137745","messageId":"7veij95ss2.fsf@alter.siamese.dyndns.org","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1003241656010.694@xanadu.home","subject":"Re: [PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-24T21:11:41Z","receivedAt":"2010-03-24T21:11:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Wed, 24 Mar 2010, Shawn O. Pearce wrote:\n>\n>> Nicolas Pitre wrote:\n>> > Note: there was a dubious usage of fd when calling release_pack_memory() \n>> > in xmmap() which is now removed.\n>> ...\n>> > @@ -100,7 +112,7 @@ void *xmmap(void *start, size_t length,\n>> > \tif (ret == MAP_FAILED) {\n>> > \t\tif (!length)\n>> > \t\t\treturn NULL;\n>> > -\t\trelease_pack_memory(length, fd);\n>> > +\t\ttry_to_free_routine(length);\n>> \n>> This isn't dubious! The fd passed here is to prevent the pack\n>> release code from closing this fd right before we try to mmap it.\n>> Its an actual bug fix that I had to write years ago, check the\n>> history of that section of code... :-)\n>\n> Argh. My bad. I somehow thought that fd was the actual pack to free \n> when specified. Let's drop the very last hunk of the patch then. \n> xmmap() is certainly not going to be invoked concurrently to the rest of \n> sha1_file.c in a separate thread.\n>\n> Junio: I suppose you don't need me to resend?\n\nWill just drop the last hunk. Thanks, both.\n"},{"id":"137747","messageId":"7vsk7p4df0.fsf@alter.siamese.dyndns.org","threadId":"23152","inReplyTo":"20100324202814.GA24830@spearce.org","subject":"Re: [PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-24T21:28:51Z","receivedAt":"2010-03-24T21:28:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n>> @@ -100,7 +112,7 @@ void *xmmap(void *start, size_t length,\n>> \tif (ret == MAP_FAILED) {\n>> \t\tif (!length)\n>> \t\t\treturn NULL;\n>> -\t\trelease_pack_memory(length, fd);\n>> +\t\ttry_to_free_routine(length);\n>\n> This isn't dubious! The fd passed here is to prevent the pack release\n> code from closing this fd right before we try to mmap it. Its an actual\n> bug fix that I had to write years ago, check the history of that section\n> of code... :-)\n\nA tangent.\n\nI thought that it incidentally might be a good example for the \"line-mode\"\nlog that has been discussed recently to follow the history of this code,\nbut this turns out to be too easy:\n\n$ git blame -C -L'/^void \\*xmmap/,/^}/' wrapper.c\n\ndirectly gives you the answer. d1efefa4 explains why this passes fd\nrather well.\n"},{"id":"137945","messageId":"4c8ef71003270626y45685e69j28ccb8a8738b9083@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1003241613020.694@xanadu.home","subject":"Re: [PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-27T13:26:20Z","receivedAt":"2010-03-27T13:26:20Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Mar 24, 2010 at 21:22, Nicolas Pitre wrote:\n> By providing a hook for the routine responsible for trying to free some\n> memory on malloc failure, we can ensure that the  called routine is\n> protected by the appropriate locks when threads are in play.\n>\n> The obvious offender here was pack-objects which was calling xmalloc()\n> within threads while release_pack_memory() is not thread safe.\n>\n> Signed-off-by: Nicolas Pitre \n> ---\n>\n> On Wed, 24 Mar 2010, Shawn Pearce wrote:\n>\n>> On Wed, Mar 24, 2010 at 11:54 AM, Nicolas Pitre wrote:\n>> > Another solution could be for xmalloc() to use a function pointer for\n>> > the method to use on malloc error path, which would default to a\n>> > function calling release_pack_memory(size, -1).  Then pack-objects.c\n>> > would override the default with its own to acquire the read_mutex around\n>> > the call to release_pack_memory().  That is probably the easiest\n>> > solution for now.\n>>\n>> Yea, that sounds like the most reasonable solution right now.\n>\n> So here it is.\n>\n> Note: there was a dubious usage of fd when calling release_pack_memory()\n> in xmmap() which is now removed.\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 9780258..65f797f 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1522,6 +1522,13 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n>\n>  #ifndef NO_PTHREADS\n>\n> +static void try_to_free_from_threads(size_t size)\n> +{\n> +       read_lock();\n> +       release_pack_memory(size, -1);\n> +       read_unlock();\n> +}\n> +\n\nWill this really work in all cases? In the find_deltas -> try_delta ->\nread_sha1_file -> ... -> xmalloc call path, the mutex is already\nlocked when we get to xmalloc. As the mutex is of the default type\n(NULL is passed as the mutex attribute argument to\npthread_mutex_init), it is undefined behaviour to lock the mutex again\n(see http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_mutexattr_gettype.html\n)\n\nI just realised that builtin/grep.c also needs a fix for its use of xmalloc.\n\n- Fredrik\n"},{"id":"137958","messageId":"alpine.LFD.2.00.1003271035360.694@xanadu.home","threadId":"23152","inReplyTo":"4c8ef71003270626y45685e69j28ccb8a8738b9083@mail.gmail.com","subject":"Re: [PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-03-27T18:59:41Z","receivedAt":"2010-03-27T18:59:41Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sat, 27 Mar 2010, Fredrik Kuivinen wrote:\n\n> On Wed, Mar 24, 2010 at 21:22, Nicolas Pitre wrote:\n> > +static void try_to_free_from_threads(size_t size)\n> > +{\n> > +       read_lock();\n> > +       release_pack_memory(size, -1);\n> > +       read_unlock();\n> > +}\n> > +\n> \n> Will this really work in all cases? In the find_deltas -> try_delta ->\n> read_sha1_file -> ... -> xmalloc call path, the mutex is already\n> locked when we get to xmalloc.\n\nYou're right. Damn.\n\n\nNicolas\n"},{"id":"138262","messageId":"s2t4c8ef71003302357x5e9defa1l4dbde6391d533ca5@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1003271035360.694@xanadu.home","subject":"Re: [PATCH] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-03-31T06:57:55Z","receivedAt":"2010-03-31T06:57:55Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Sat, Mar 27, 2010 at 19:59, Nicolas Pitre wrote:\n> On Sat, 27 Mar 2010, Fredrik Kuivinen wrote:\n>\n>> On Wed, Mar 24, 2010 at 21:22, Nicolas Pitre wrote:\n>> > +static void try_to_free_from_threads(size_t size)\n>> > +{\n>> > +       read_lock();\n>> > +       release_pack_memory(size, -1);\n>> > +       read_unlock();\n>> > +}\n>> > +\n>>\n>> Will this really work in all cases? In the find_deltas -> try_delta ->\n>> read_sha1_file -> ... -> xmalloc call path, the mutex is already\n>> locked when we get to xmalloc.\n>\n> You're right.  Damn.\n\nA simple fix is to make it a recursive mutex instead. This will work\nwith a minimal change in win32 as well as the CRITICAL_SECTION type is\nrecursive.\n\nI guess the downside is that the locking potentially gets slightly slower.\n\n\n- Fredrik\n"},{"id":"138798","messageId":"alpine.LFD.2.00.1004062152260.7232@xanadu.home","threadId":"23152","inReplyTo":"4c8ef71003270626y45685e69j28ccb8a8738b9083@mail.gmail.com","subject":"[PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-07T02:57:58Z","receivedAt":"2010-04-07T02:57:58Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"By providing a hook for the routine responsible for trying to free some\nmemory on malloc failure, we can ensure that the called routine is\nprotected by the appropriate locks when threads are in play.\n\nThe obvious offender here was pack-objects which was calling xmalloc()\nwithin threads while release_pack_memory() is not thread safe.\n\nTo avoid a deadlock if try_to_free_from_threads() is called while\nread_lock is already locked within the same thread (may happen through\nthe read_sha1_file() path), a simple mutex ownership is added. This \ncould have been handled automatically with the PTHREAD_MUTEX_RECURSIVE \ntype but the Windows pthread emulation would get much more complex.\n\nSigned-off-by: Nicolas Pitre \n---\n\nOn Sat, 27 Mar 2010, Fredrik Kuivinen wrote:\n\n> > +static void try_to_free_from_threads(size_t size)\n> > +{\n> > +       read_lock();\n> > +       release_pack_memory(size, -1);\n> > +       read_unlock();\n> > +}\n> > +\n> \n> Will this really work in all cases? In the find_deltas -> try_delta ->\n> read_sha1_file -> ... -> xmalloc call path, the mutex is already\n> locked when we get to xmalloc.\n\nSo here's a fixed version.\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 9780258..d3ac41f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1211,8 +1211,17 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,\n #ifndef NO_PTHREADS\n \n static pthread_mutex_t read_mutex;\n-#define read_lock()\t\tpthread_mutex_lock(&read_mutex)\n-#define read_unlock()\t\tpthread_mutex_unlock(&read_mutex)\n+static pthread_t read_mutex_owner;\n+#define read_lock() \\\n+\tdo { \\\n+\t\tpthread_mutex_lock(&read_mutex); \\\n+\t\tread_mutex_owner = pthread_self(); \\\n+\t} while (0)\n+#define read_unlock() \\\n+\tdo { \\\n+\t\tmemset(&read_mutex_owner, 0, sizeof(read_mutex_owner)); \\\n+\t\tpthread_mutex_unlock(&read_mutex); \\\n+\t} while (0)\n \n static pthread_mutex_t cache_mutex;\n #define cache_lock()\t\tpthread_mutex_lock(&cache_mutex)\n@@ -1522,6 +1531,16 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n \n #ifndef NO_PTHREADS\n \n+static void try_to_free_from_threads(size_t size)\n+{\n+\tint self = pthread_equal(read_mutex_owner, pthread_self());\n+\tif (!self)\n+\t\tread_lock();\n+\trelease_pack_memory(size, -1);\n+\tif (!self)\n+\t\tread_unlock();\n+}\n+\n /*\n * The main thread waits on the condition that (at least) one of the workers\n * has stopped working (which is indicated in the .working member of\n@@ -1556,10 +1575,12 @@ static void init_threaded_search(void)\n \tpthread_mutex_init(&cache_mutex, NULL);\n \tpthread_mutex_init(&progress_mutex, NULL);\n \tpthread_cond_init(&progress_cond, NULL);\n+\tset_try_to_free_routine(try_to_free_from_threads);\n }\n \n static void cleanup_threaded_search(void)\n {\n+\tset_try_to_free_routine(NULL);\n \tpthread_cond_destroy(&progress_cond);\n \tpthread_mutex_destroy(&read_mutex);\n \tpthread_mutex_destroy(&cache_mutex);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b56c297..4ee8f86 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -357,6 +357,8 @@ static inline void *gitmempcpy(void *dest, const void *src, size_t n)\n \n extern void release_pack_memory(size_t, int);\n \n+extern void set_try_to_free_routine(void (*routine)(size_t));\n+\n extern char *xstrdup(const char *str);\n extern void *xmalloc(size_t size);\n extern void *xmallocz(size_t size);\ndiff --git a/wrapper.c b/wrapper.c\nindex 10a6750..58201b6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -3,11 +3,23 @@\n */\n #include \"cache.h\"\n \n+static void try_to_free_builtin(size_t size)\n+{\n+\trelease_pack_memory(size, -1);\n+}\n+\n+static void (*try_to_free_routine)(size_t size) = try_to_free_builtin;\n+\n+void set_try_to_free_routine(void (*routine)(size_t))\n+{\n+\ttry_to_free_routine = (routine) ? routine : try_to_free_builtin;\n+}\n+\n char *xstrdup(const char *str)\n {\n \tchar *ret = strdup(str);\n \tif (!ret) {\n-\t\trelease_pack_memory(strlen(str) + 1, -1);\n+\t\ttry_to_free_routine(strlen(str) + 1);\n \t\tret = strdup(str);\n \t\tif (!ret)\n \t\t\tdie(\"Out of memory, strdup failed\");\n@@ -21,7 +33,7 @@ void *xmalloc(size_t size)\n \tif (!ret && !size)\n \t\tret = malloc(1);\n \tif (!ret) {\n-\t\trelease_pack_memory(size, -1);\n+\t\ttry_to_free_routine(size);\n \t\tret = malloc(size);\n \t\tif (!ret && !size)\n \t\t\tret = malloc(1);\n@@ -67,7 +79,7 @@ void *xrealloc(void *ptr, size_t size)\n \tif (!ret && !size)\n \t\tret = realloc(ptr, 1);\n \tif (!ret) {\n-\t\trelease_pack_memory(size, -1);\n+\t\ttry_to_free_routine(size);\n \t\tret = realloc(ptr, size);\n \t\tif (!ret && !size)\n \t\t\tret = realloc(ptr, 1);\n@@ -83,7 +95,7 @@ void *xcalloc(size_t nmemb, size_t size)\n \tif (!ret && (!nmemb || !size))\n \t\tret = calloc(1, 1);\n \tif (!ret) {\n-\t\trelease_pack_memory(nmemb * size, -1);\n+\t\ttry_to_free_routine(nmemb * size);\n \t\tret = calloc(nmemb, size);\n \t\tif (!ret && (!nmemb || !size))\n \t\t\tret = calloc(1, 1);\n"},{"id":"138799","messageId":"20100407031655.GA7156@spearce.org","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1004062152260.7232@xanadu.home","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-07T03:16:55Z","receivedAt":"2010-04-07T03:16:55Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> To avoid a deadlock if try_to_free_from_threads() is called while\n> read_lock is already locked within the same thread (may happen through\n> the read_sha1_file() path), a simple mutex ownership is added. This \n> could have been handled automatically with the PTHREAD_MUTEX_RECURSIVE \n> type but the Windows pthread emulation would get much more complex.\n...\n> +static void try_to_free_from_threads(size_t size)\n> +{\n> +\tint self = pthread_equal(read_mutex_owner, pthread_self());\n> +\tif (!self)\n> +\t\tread_lock();\n> +\trelease_pack_memory(size, -1);\n> +\tif (!self)\n> +\t\tread_unlock();\n> +}\n\nIs there any concern that a partially unset read_mutex_owner might\nlook like the current thread's identity?\n\nThat is, memset() can be setting the bytes one by one. If the lock\nis being released we might observe the current owner as ourselves\nif we see only part of that release, and our identity is the same\nas another thread, only with the lower-address bytes unset.\n\n-- \nShawn.\n"},{"id":"138800","messageId":"alpine.LFD.2.00.1004070043450.7232@xanadu.home","threadId":"23152","inReplyTo":"20100407031655.GA7156@spearce.org","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-07T04:51:12Z","receivedAt":"2010-04-07T04:51:12Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 6 Apr 2010, Shawn O. Pearce wrote:\n\n> Nicolas Pitre wrote:\n> > To avoid a deadlock if try_to_free_from_threads() is called while\n> > read_lock is already locked within the same thread (may happen through\n> > the read_sha1_file() path), a simple mutex ownership is added. This \n> > could have been handled automatically with the PTHREAD_MUTEX_RECURSIVE \n> > type but the Windows pthread emulation would get much more complex.\n> ...\n> > +static void try_to_free_from_threads(size_t size)\n> > +{\n> > +\tint self = pthread_equal(read_mutex_owner, pthread_self());\n> > +\tif (!self)\n> > +\t\tread_lock();\n> > +\trelease_pack_memory(size, -1);\n> > +\tif (!self)\n> > +\t\tread_unlock();\n> > +}\n> \n> Is there any concern that a partially unset read_mutex_owner might\n> look like the current thread's identity?\n> \n> That is, memset() can be setting the bytes one by one. If the lock\n> is being released we might observe the current owner as ourselves\n> if we see only part of that release, and our identity is the same\n> as another thread, only with the lower-address bytes unset.\n\nIn practice memset() will optimize the memory access by using words and \nno bytes. But in theory this is not guaranteed. The solution for this \nwould be to have yet another mutex just to protect the read_mutex \nhownership information modifications in order to make it atomic to \npotential readers. That is becoming ugly for a feature (the freeing of \npack data) that is not supposed to be the common case.\n\n\nNicolas\n"},{"id":"138802","messageId":"7vk4sjg7nb.fsf@alter.siamese.dyndns.org","threadId":"23152","inReplyTo":"20100407031655.GA7156@spearce.org","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-07T05:21:12Z","receivedAt":"2010-04-07T05:21:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n>> +static void try_to_free_from_threads(size_t size)\n>> +{\n>> +\tint self = pthread_equal(read_mutex_owner, pthread_self());\n>> +\tif (!self)\n>> +\t\tread_lock();\n>> +\trelease_pack_memory(size, -1);\n>> +\tif (!self)\n>> +\t\tread_unlock();\n>> +}\n>\n> Is there any concern that a partially unset read_mutex_owner might\n> look like the current thread's identity?\n>\n> That is, memset() can be setting the bytes one by one. If the lock\n> is being released we might observe the current owner as ourselves\n> if we see only part of that release, and our identity is the same\n> as another thread, only with the lower-address bytes unset.\n\nYuck. I hope that it doesn't mean we need another mutex to protect the\nowner data.\n"},{"id":"138822","messageId":"r2xec874dac1004070529p3d21d23z533e471636194c00@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1004070043450.7232@xanadu.home","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-07T12:29:15Z","receivedAt":"2010-04-07T12:29:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, Apr 6, 2010 at 9:51 PM, Nicolas Pitre wrote:\n> On Tue, 6 Apr 2010, Shawn O. Pearce wrote:\n>> Nicolas Pitre wrote:\n>> > To avoid a deadlock if try_to_free_from_threads() is called while\n>> > read_lock is already locked within the same thread (may happen through\n>> > the read_sha1_file() path), a simple mutex ownership is added. This\n>> > could have been handled automatically with the PTHREAD_MUTEX_RECURSIVE\n>> > type but the Windows pthread emulation would get much more complex.\n>> ...\n>> > +static void try_to_free_from_threads(size_t size)\n>> > +{\n>> > +   int self = pthread_equal(read_mutex_owner, pthread_self());\n>> > +   if (!self)\n>> > +           read_lock();\n>> > +   release_pack_memory(size, -1);\n>> > +   if (!self)\n>> > +           read_unlock();\n>> > +}\n>>\n>> Is there any concern that a partially unset read_mutex_owner might\n>> look like the current thread's identity?\n>>\n>> That is, memset() can be setting the bytes one by one.  If the lock\n>> is being released we might observe the current owner as ourselves\n>> if we see only part of that release, and our identity is the same\n>> as another thread, only with the lower-address bytes unset.\n>\n> In practice memset() will optimize the memory access by using words and\n> no bytes.  But in theory this is not guaranteed.  The solution for this\n> would be to have yet another mutex just to protect the read_mutex\n> hownership information modifications in order to make it atomic to\n> potential readers.  That is becoming ugly for a feature (the freeing of\n> pack data) that is not supposed to be the common case.\n\nMulti-threaded programming is hard. Its never easy to get it right.\nWe had really excellent reasons for avoiding multiple threads in the\nearly days of Git. We still have those excellent reasons, but we have\nbeen pushing more and more into these async threads to support\nwindows, and now its making us realize we never really thought about\nthis stuff very much.\n\nYou mentioned avoiding a recursive mutex only because windows\nemulation doesn't have support for it. But that's exactly what we\nneed here. Shouldn't windows have a recursive mutex object that can\njust be used inside of the emulation layer when we really need a\nrecursive mutex?\n\n-- \nShawn.\n"},{"id":"138825","messageId":"alpine.LFD.2.00.1004070859540.7232@xanadu.home","threadId":"23152","inReplyTo":"r2xec874dac1004070529p3d21d23z533e471636194c00@mail.gmail.com","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-07T13:17:04Z","receivedAt":"2010-04-07T13:17:04Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 7 Apr 2010, Shawn Pearce wrote:\n\n> On Tue, Apr 6, 2010 at 9:51 PM, Nicolas Pitre wrote:\n> > In practice memset() will optimize the memory access by using words and\n> > no bytes.  But in theory this is not guaranteed.  The solution for this\n> > would be to have yet another mutex just to protect the read_mutex\n> > hownership information modifications in order to make it atomic to\n> > potential readers.  That is becoming ugly for a feature (the freeing of\n> > pack data) that is not supposed to be the common case.\n> \n> Multi-threaded programming is hard. Its never easy to get it right.\n\nIndeed. But in this case what makes it harder is the willingness to \nstick to standard APIs.\n\n> We had really excellent reasons for avoiding multiple threads in the\n> early days of Git. We still have those excellent reasons, but we have\n> been pushing more and more into these async threads to support\n> windows, and now its making us realize we never really thought about\n> this stuff very much.\n\nWell, pack-objects was \"broken\" even without Windows in the picture. \nWhat I'm trying to fix here is not Windows specific (although the thread \nAPI limitations are).\n\n> You mentioned avoiding a recursive mutex only because windows\n> emulation doesn't have support for it. But that's exactly what we\n> need here. Shouldn't windows have a recursive mutex object that can\n> just be used inside of the emulation layer when we really need a\n> recursive mutex?\n\nMaybe. That would in fact just mean pushing the double mutex issue into \nthe pthread emulation instead of having it outside it. This would \nimpact performances for all mutexes although only one instance of them \ncurrently require a recursive behavior.\n\nYet, the memset() issue comes up only because pthread_t is meant to be \nan opaque type. The only information we would need here is the actual \nthread ID as returned by gettid() on Linux or GetCurrentThreadId() on \nWindows, and then the read_mutex_owner could be a simple atomically \nmodifiable integer. But what about other pthread-capable non Linux \nsystems?\n\n\nNicolas\n"},{"id":"138832","messageId":"w2kec874dac1004070730rd1e5c149x88a7d6b4b649792f@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1004070859540.7232@xanadu.home","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-04-07T14:30:09Z","receivedAt":"2010-04-07T14:30:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Apr 7, 2010 at 6:17 AM, Nicolas Pitre wrote:\n> Maybe.  That would in fact just mean pushing the double mutex issue into\n> the pthread emulation instead of having it outside it.  This would\n> impact performances for all mutexes although only one instance of them\n> currently require a recursive behavior.\n\nBut we know when the mutex is created whether or not it needs\nrecursive support. So in the Windows emulation we may need to\nallocate two mutexes for each pthread_mutex_t storage-wise, but we\ndon't necessarily need to use that owner thread mutex on every\nlock/unlock request, do we?\n\n> Yet, the memset() issue comes up only because pthread_t is meant to be\n> an opaque type.  The only information we would need here is the actual\n> thread ID as returned by gettid() on Linux or GetCurrentThreadId() on\n> Windows, and then the read_mutex_owner could be a simple atomically\n> modifiable integer.  But what about other pthread-capable non Linux\n> systems?\n\nIndeed. If Windows threads are atomic words, then actually you don't\nneed that double mutex in the emulation layer, and can instead use the\natomic word to determine ownership. Which makes this entire debate\nsomewhat moot, doesn't it?\n\nUse a standard recursive pthread mutex, and implement recursive\nsupport in the emulation layer using the atomic word holding a\nGetCurrentThreadId() result. We don't need to worry about how\npthread_t is stored on any particular system, the thread library will\ndo it for us.\n\n-- \nShawn.\n"},{"id":"138835","messageId":"20100407144555.GA23911@fredrik-laptop","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1004070859540.7232@xanadu.home","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-04-07T14:45:55Z","receivedAt":"2010-04-07T14:45:55Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Apr 07, 2010 at 09:17:04AM -0400, Nicolas Pitre wrote:\n> On Wed, 7 Apr 2010, Shawn Pearce wrote:\n> > You mentioned avoiding a recursive mutex only because windows\n> > emulation doesn't have support for it. But that's exactly what we\n> > need here. Shouldn't windows have a recursive mutex object that can\n> > just be used inside of the emulation layer when we really need a\n> > recursive mutex?\n> \n> Maybe. That would in fact just mean pushing the double mutex issue into \n> the pthread emulation instead of having it outside it. This would \n> impact performances for all mutexes although only one instance of them \n> currently require a recursive behavior.\n\nAs I mentioned in another mail in this thread, our mutex\nimplementation on WIN32 already is recursive. It is implemented on top\nof the CRITICAL_SECTION type, which is recursive. See\nhttp://msdn.microsoft.com/en-us/library/ms682530%28VS.85%29.aspx\n\nWe only need something like the following (on top of Nico's previous\npatch). Warning: It hasn't even been compile tested on WIN32.\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 65f797f..19e42cf 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1559,7 +1559,7 @@ static pthread_cond_t progress_cond;\n */\n static void init_threaded_search(void)\n {\n-\tpthread_mutex_init(&read_mutex, NULL);\n+\tinit_recursive_mutex(&read_mutex);\n \tpthread_mutex_init(&cache_mutex, NULL);\n \tpthread_mutex_init(&progress_mutex, NULL);\n \tpthread_cond_init(&progress_cond, NULL);\ndiff --git a/thread-utils.c b/thread-utils.c\nindex 4f9c829..3c8d817 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \n \n #if defined(hpux) || defined(__hpux) || defined(_hpux)\n # include \n@@ -43,3 +44,24 @@ int online_cpus(void)\n \n \treturn 1;\n }\n+\n+int init_recursive_mutex(pthread_mutex_t *m)\n+{\n+#ifdef _WIN32\n+\t/* The mutexes in the WIN32 pthreads emulation layer are\n+\t * recursive, so we don't have to do anything extra here. */\n+\treturn pthread_mutex_init(m, NULL);\n+#else\n+\tpthread_mutexattr_t a;\n+\tint ret;\n+\tif (pthread_mutexattr_init(&a))\n+\t\tdie(\"pthread_mutexattr_init failed: %s\", strerror(errno));\n+\n+\tif (pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE))\n+\t\tdie(\"pthread_mutexattr_settype failed: %s\", strerror(errno));\n+\n+\tret = pthread_mutex_init(m, &a);\n+\tpthread_mutexattr_destroy(&a);\n+\treturn ret;\n+#endif\n+}\ndiff --git a/thread-utils.h b/thread-utils.h\nindex cce4b77..1727a03 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -2,5 +2,6 @@\n #define THREAD_COMPAT_H\n \n extern int online_cpus(void);\n+extern int init_recursive_mutex(pthread_mutex_t*);\n \n #endif /* THREAD_COMPAT_H */\n"},{"id":"138836","messageId":"alpine.LFD.2.00.1004071044070.7232@xanadu.home","threadId":"23152","inReplyTo":"w2kec874dac1004070730rd1e5c149x88a7d6b4b649792f@mail.gmail.com","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-07T14:47:01Z","receivedAt":"2010-04-07T14:47:01Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 7 Apr 2010, Shawn Pearce wrote:\n\n> On Wed, Apr 7, 2010 at 6:17 AM, Nicolas Pitre wrote:\n> > Yet, the memset() issue comes up only because pthread_t is meant to be\n> > an opaque type.  The only information we would need here is the actual\n> > thread ID as returned by gettid() on Linux or GetCurrentThreadId() on\n> > Windows, and then the read_mutex_owner could be a simple atomically\n> > modifiable integer.  But what about other pthread-capable non Linux\n> > systems?\n> \n> Indeed. If Windows threads are atomic words, then actually you don't\n> need that double mutex in the emulation layer, and can instead use the\n> atomic word to determine ownership. Which makes this entire debate\n> somewhat moot, doesn't it?\n\nWell, indeed.\n\n\nNicolas\n"},{"id":"138841","messageId":"alpine.LFD.2.00.1004071103341.7232@xanadu.home","threadId":"23152","inReplyTo":"20100407144555.GA23911@fredrik-laptop","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-07T15:08:41Z","receivedAt":"2010-04-07T15:08:41Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 7 Apr 2010, Fredrik Kuivinen wrote:\n\n> As I mentioned in another mail in this thread, our mutex\n> implementation on WIN32 already is recursive. It is implemented on top\n> of the CRITICAL_SECTION type, which is recursive. See\n> http://msdn.microsoft.com/en-us/library/ms682530%28VS.85%29.aspx\n\nAhhhh. Goodie.\n\n> We only need something like the following (on top of Nico's previous\n> patch). Warning: It hasn't even been compile tested on WIN32.\n> \n[...]\n> diff --git a/thread-utils.c b/thread-utils.c\n> index 4f9c829..3c8d817 100644\n> --- a/thread-utils.c\n> +++ b/thread-utils.c\n> @@ -1,4 +1,5 @@\n> #include \"cache.h\"\n> +#include \n\nThis will fail compilation on Windows surely?\n\n> #if defined(hpux) || defined(__hpux) || defined(_hpux)\n> # include \n> @@ -43,3 +44,24 @@ int online_cpus(void)\n> \n> \treturn 1;\n> }\n> +\n> +int init_recursive_mutex(pthread_mutex_t *m)\n> +{\n> +#ifdef _WIN32\n> +\t/* The mutexes in the WIN32 pthreads emulation layer are\n> +\t * recursive, so we don't have to do anything extra here. */\n> +\treturn pthread_mutex_init(m, NULL);\n> +#else\n> +\tpthread_mutexattr_t a;\n> +\tint ret;\n> +\tif (pthread_mutexattr_init(&a))\n> +\t\tdie(\"pthread_mutexattr_init failed: %s\", strerror(errno));\n> +\n> +\tif (pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE))\n> +\t\tdie(\"pthread_mutexattr_settype failed: %s\", strerror(errno));\n> +\n> +\tret = pthread_mutex_init(m, &a);\n> +\tpthread_mutexattr_destroy(&a);\n> +\treturn ret;\n\nAre you sure the pthread_mutexattr_t object can be destroyed even if the \nmutex is still in use? Is the attribute object \"attached\" to the mutex \nor merely used as a template?\n\n\nNicolas\n"},{"id":"138846","messageId":"i2sfabb9a1e1004070827m2745f9ccz5976664852eabd3b@mail.gmail.com","threadId":"23152","inReplyTo":"20100407144555.GA23911@fredrik-laptop","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-04-07T15:27:57Z","receivedAt":"2010-04-07T15:27:57Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Apr 7, 2010 at 09:45, Fredrik Kuivinen wrote:\n> +               die(\"pthread_mutexattr_settype failed: %s\", strerror(errno));\n\nCan't you just die_errno here?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"138851","messageId":"v2t4c8ef71004070913r2de3c8car31f39a2ab7aa6d15@mail.gmail.com","threadId":"23152","inReplyTo":"alpine.LFD.2.00.1004071103341.7232@xanadu.home","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-04-07T16:13:18Z","receivedAt":"2010-04-07T16:13:18Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Apr 7, 2010 at 17:08, Nicolas Pitre wrote:\n> On Wed, 7 Apr 2010, Fredrik Kuivinen wrote:\n>> diff --git a/thread-utils.c b/thread-utils.c\n>> index 4f9c829..3c8d817 100644\n>> --- a/thread-utils.c\n>> +++ b/thread-utils.c\n>> @@ -1,4 +1,5 @@\n>>  #include \"cache.h\"\n>> +#include \n>\n> This will fail compilation on Windows surely?\n\nI think it will work. We use \"#include \" in builtin/grep.c,\nbuiltin/pack-objects.c, and preload-index.c already.\n\n>>  #if defined(hpux) || defined(__hpux) || defined(_hpux)\n>>  #  include \n>> @@ -43,3 +44,24 @@ int online_cpus(void)\n>>\n>>       return 1;\n>>  }\n>> +\n>> +int init_recursive_mutex(pthread_mutex_t *m)\n>> +{\n>> +#ifdef _WIN32\n>> +     /* The mutexes in the WIN32 pthreads emulation layer are\n>> +      * recursive, so we don't have to do anything extra here. */\n>> +     return pthread_mutex_init(m, NULL);\n>> +#else\n>> +     pthread_mutexattr_t a;\n>> +     int ret;\n>> +     if (pthread_mutexattr_init(&a))\n>> +             die(\"pthread_mutexattr_init failed: %s\", strerror(errno));\n>> +\n>> +     if (pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE))\n>> +             die(\"pthread_mutexattr_settype failed: %s\", strerror(errno));\n>> +\n>> +     ret = pthread_mutex_init(m, &a);\n>> +     pthread_mutexattr_destroy(&a);\n>> +     return ret;\n>\n> Are you sure the pthread_mutexattr_t object can be destroyed even if the\n> mutex is still in use?  Is the attribute object \"attached\" to the mutex\n> or merely used as a template?\n\nIt is safe. See\nhttp://www.opengroup.org/onlinepubs/009695399/functions/pthread_mutexattr_init.html\n\n- Fredrik\n"},{"id":"138852","messageId":"w2w4c8ef71004070915td320f46as66da08712c6aaa91@mail.gmail.com","threadId":"23152","inReplyTo":"i2sfabb9a1e1004070827m2745f9ccz5976664852eabd3b@mail.gmail.com","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-04-07T16:15:55Z","receivedAt":"2010-04-07T16:15:55Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Apr 7, 2010 at 17:27, Sverre Rabbelier wrote:\n> Heya,\n>\n> On Wed, Apr 7, 2010 at 09:45, Fredrik Kuivinen wrote:\n>> +               die(\"pthread_mutexattr_settype failed: %s\", strerror(errno));\n>\n> Can't you just die_errno here?\n\nAh, yes you are right. Thanks.\n\n\n- Fredrik\n"},{"id":"138853","messageId":"7v8w8zdypf.fsf@alter.siamese.dyndns.org","threadId":"23152","inReplyTo":"20100407144555.GA23911@fredrik-laptop","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-07T16:17:16Z","receivedAt":"2010-04-07T16:17:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Kuivinen writes:\n\n> As I mentioned in another mail in this thread, our mutex\n> implementation on WIN32 already is recursive.\n\nGood ;-)\n"},{"id":"138854","messageId":"n2n40aa078e1004070944sc137aef3yfc5967d517860192@mail.gmail.com","threadId":"23152","inReplyTo":"v2t4c8ef71004070913r2de3c8car31f39a2ab7aa6d15@mail.gmail.com","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-04-07T16:44:16Z","receivedAt":"2010-04-07T16:44:16Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Apr 7, 2010 at 6:13 PM, Fredrik Kuivinen wrote:\n> On Wed, Apr 7, 2010 at 17:08, Nicolas Pitre wrote:\n>> On Wed, 7 Apr 2010, Fredrik Kuivinen wrote:\n>>> diff --git a/thread-utils.c b/thread-utils.c\n>>> index 4f9c829..3c8d817 100644\n>>> --- a/thread-utils.c\n>>> +++ b/thread-utils.c\n>>> @@ -1,4 +1,5 @@\n>>>  #include \"cache.h\"\n>>> +#include \n>>\n>> This will fail compilation on Windows surely?\n>\n> I think it will work. We use \"#include \" in builtin/grep.c,\n> builtin/pack-objects.c, and preload-index.c already.\n>\n\nStill, isn't this REALLY the kind of stuff that usually goes in\ngit-compat-util.h? I'm not asking you to change it, I'm just a bit\npuzzled that the other pthread-clients did it this way...\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"138871","messageId":"alpine.LFD.2.00.1004071431000.7232@xanadu.home","threadId":"23152","inReplyTo":"v2t4c8ef71004070913r2de3c8car31f39a2ab7aa6d15@mail.gmail.com","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-04-07T18:37:13Z","receivedAt":"2010-04-07T18:37:13Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 7 Apr 2010, Fredrik Kuivinen wrote:\n\n> On Wed, Apr 7, 2010 at 17:08, Nicolas Pitre wrote:\n> > On Wed, 7 Apr 2010, Fredrik Kuivinen wrote:\n> >> diff --git a/thread-utils.c b/thread-utils.c\n> >> index 4f9c829..3c8d817 100644\n> >> --- a/thread-utils.c\n> >> +++ b/thread-utils.c\n> >> @@ -1,4 +1,5 @@\n> >>  #include \"cache.h\"\n> >> +#include \n> >\n> > This will fail compilation on Windows surely?\n> \n> I think it will work. We use \"#include \" in builtin/grep.c,\n> builtin/pack-objects.c, and preload-index.c already.\n\nTrue indeed.\n\n> >>  #if defined(hpux) || defined(__hpux) || defined(_hpux)\n> >>  #  include \n> >> @@ -43,3 +44,24 @@ int online_cpus(void)\n> >>\n> >>       return 1;\n> >>  }\n> >> +\n> >> +int init_recursive_mutex(pthread_mutex_t *m)\n> >> +{\n> >> +#ifdef _WIN32\n> >> +     /* The mutexes in the WIN32 pthreads emulation layer are\n> >> +      * recursive, so we don't have to do anything extra here. */\n> >> +     return pthread_mutex_init(m, NULL);\n> >> +#else\n> >> +     pthread_mutexattr_t a;\n> >> +     int ret;\n> >> +     if (pthread_mutexattr_init(&a))\n> >> +             die(\"pthread_mutexattr_init failed: %s\", strerror(errno));\n> >> +\n> >> +     if (pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE))\n> >> +             die(\"pthread_mutexattr_settype failed: %s\", strerror(errno));\n> >> +\n> >> +     ret = pthread_mutex_init(m, &a);\n> >> +     pthread_mutexattr_destroy(&a);\n> >> +     return ret;\n> >\n> > Are you sure the pthread_mutexattr_t object can be destroyed even if the\n> > mutex is still in use?  Is the attribute object \"attached\" to the mutex\n> > or merely used as a template?\n> \n> It is safe. See\n> http://www.opengroup.org/onlinepubs/009695399/functions/pthread_mutexattr_init.html\n\nOK. ACK to your patch then.\n\n\nNicolas\n"},{"id":"138875","messageId":"201004072049.08819.j6t@kdbg.org","threadId":"23152","inReplyTo":"20100407144555.GA23911@fredrik-laptop","subject":"Re: [PATCH v2] Make xmalloc and xrealloc thread-safe","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-04-07T18:49:08Z","receivedAt":"2010-04-07T18:49:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Mittwoch, 7. April 2010, Fredrik Kuivinen wrote:\n> As I mentioned in another mail in this thread, our mutex\n> implementation on WIN32 already is recursive. It is implemented on top\n> of the CRITICAL_SECTION type, which is recursive. See\n> http://msdn.microsoft.com/en-us/library/ms682530%28VS.85%29.aspx\n\nVery true!\n\n> +\tif (pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE))\n\nI wonder how many pthreads implementations there are that do not support \nrecursive mutexes...\n\n-- Hannes\n"},{"id":"138946","messageId":"4BBD829B.8040700@viscovery.net","threadId":"23152","inReplyTo":"20100407144555.GA23911@fredrik-laptop","subject":"[PATCH] Thread-safe xmalloc and xrealloc needs a recursive mutex","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-04-08T07:15:39Z","receivedAt":"2010-04-08T07:15:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nThe mutex used to protect object access (read_mutex) may need to be\nacquired recursively. Introduce init_recursive_mutex() helper function\nin thread-utils.c that constructs a mutex with the PHREAD_MUTEX_RECURSIVE\nattribute.\n\npthread_mutex_init() emulation on Win32 is already recursive as it is\nimplemented on top of the CRITICAL_SECTION type, which is recursive.\n\n http://msdn.microsoft.com/en-us/library/ms682530%28VS.85%29.aspx\n\nAdd do-nothing compatibility wrappers for pthread_mutexattr* functions.\n\nInitial-version-by: Fredrik Kuivinen \nSigned-off-by: Johannes Sixt \n---\nAm 4/7/2010 16:45, schrieb Fredrik Kuivinen:\n> We only need something like the following (on top of Nico's previous\n> patch). Warning: It hasn't even been compile tested on WIN32.\n\nUnfortunately, it doesn't build. This patch replaces the tip of\nnd/malloc-threading.\n\nBTW, your uses of strerror(errno) in init_recursive_mutex() were wrong\n(pthread functions do not set errno), but it is better in any case to\navoid die() in this function.\n\n-- Hannes\n\n builtin-grep.c | 2 +-\n builtin-pack-objects.c | 4 ++--\n compat/win32/pthread.h | 8 +++++++-\n thread-utils.c | 16 ++++++++++++++++\n thread-utils.h | 1 +\n 5 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 371db0a..52137f4 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -16,8 +16,8 @@\n #include \"quote.h\"\n \n #ifndef NO_PTHREADS\n-#include \"thread-utils.h\"\n #include \n+#include \"thread-utils.h\"\n #endif\n \n static char const * const grep_usage[] = {\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 0ecc198..26fc7cd 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -18,8 +18,8 @@\n #include \"refs.h\"\n \n #ifndef NO_PTHREADS\n-#include \"thread-utils.h\"\n #include \n+#include \"thread-utils.h\"\n #endif\n \n static const char pack_usage[] =\n@@ -1586,7 +1586,7 @@ static pthread_cond_t progress_cond;\n */\n static void init_threaded_search(void)\n {\n-\tpthread_mutex_init(&read_mutex, NULL);\n+\tinit_recursive_mutex(&read_mutex);\n \tpthread_mutex_init(&cache_mutex, NULL);\n \tpthread_mutex_init(&progress_mutex, NULL);\n \tpthread_cond_init(&progress_cond, NULL);\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex c72f100..a45f8d6 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -18,11 +18,17 @@\n */\n #define pthread_mutex_t CRITICAL_SECTION\n \n-#define pthread_mutex_init(a,b) InitializeCriticalSection((a))\n+#define pthread_mutex_init(a,b) (InitializeCriticalSection((a)), 0)\n #define pthread_mutex_destroy(a) DeleteCriticalSection((a))\n #define pthread_mutex_lock EnterCriticalSection\n #define pthread_mutex_unlock LeaveCriticalSection\n \n+typedef int pthread_mutexattr_t;\n+#define pthread_mutexattr_init(a) (*(a) = 0)\n+#define pthread_mutexattr_destroy(a) do {} while (0)\n+#define pthread_mutexattr_settype(a, t) 0\n+#define PTHREAD_MUTEX_RECURSIVE 0\n+\n /*\n * Implement simple condition variable for Windows threads, based on ACE\n * implementation.\ndiff --git a/thread-utils.c b/thread-utils.c\nindex 4f9c829..589f838 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \n \n #if defined(hpux) || defined(__hpux) || defined(_hpux)\n # include \n@@ -43,3 +44,18 @@ int online_cpus(void)\n \n \treturn 1;\n }\n+\n+int init_recursive_mutex(pthread_mutex_t *m)\n+{\n+\tpthread_mutexattr_t a;\n+\tint ret;\n+\n+\tret = pthread_mutexattr_init(&a);\n+\tif (!ret) {\n+\t\tret = pthread_mutexattr_settype(&a, PTHREAD_MUTEX_RECURSIVE);\n+\t\tif (!ret)\n+\t\t\tret = pthread_mutex_init(m, &a);\n+\t\tpthread_mutexattr_destroy(&a);\n+\t}\n+\treturn ret;\n+}\ndiff --git a/thread-utils.h b/thread-utils.h\nindex cce4b77..1727a03 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -2,5 +2,6 @@\n #define THREAD_COMPAT_H\n \n extern int online_cpus(void);\n+extern int init_recursive_mutex(pthread_mutex_t*);\n \n #endif /* THREAD_COMPAT_H */\n-- \n1.7.0.3.1356.g75346\n"},{"id":"138952","messageId":"g2h4c8ef71004080142r5df32f10u66ebba19799804eb@mail.gmail.com","threadId":"23152","inReplyTo":"4BBD829B.8040700@viscovery.net","subject":"Re: [PATCH] Thread-safe xmalloc and xrealloc needs a recursive mutex","fromName":"Fredrik Kuivinen","fromEmail":"frekui@gmail.com","sentAt":"2010-04-08T08:42:28Z","receivedAt":"2010-04-08T08:42:28Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Thu, Apr 8, 2010 at 09:15, Johannes Sixt wrote:\n> From: Johannes Sixt \n>\n> The mutex used to protect object access (read_mutex) may need to be\n> acquired recursively.  Introduce init_recursive_mutex() helper function\n> in thread-utils.c that constructs a mutex with the PHREAD_MUTEX_RECURSIVE\n> attribute.\n>\n> pthread_mutex_init() emulation on Win32 is already recursive as it is\n> implemented on top of the CRITICAL_SECTION type, which is recursive.\n>\n>    http://msdn.microsoft.com/en-us/library/ms682530%28VS.85%29.aspx\n>\n> Add do-nothing compatibility wrappers for pthread_mutexattr* functions.\n>\n> Initial-version-by: Fredrik Kuivinen \n> Signed-off-by: Johannes Sixt \n> ---\n> Am 4/7/2010 16:45, schrieb Fredrik Kuivinen:\n>> We only need something like the following (on top of Nico's previous\n>> patch). Warning: It hasn't even been compile tested on WIN32.\n>\n> Unfortunately, it doesn't build. This patch replaces the tip of\n> nd/malloc-threading.\n>\n> BTW, your uses of strerror(errno) in init_recursive_mutex() were wrong\n> (pthread functions do not set errno), but it is better in any case to\n> avoid die() in this function.\n\nVery true. Thanks.\n\n- Fredrik\n"}]}