{"thread":{"id":"50255","subject":"[PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","startedAt":"2019-01-18T02:28:20Z","lastAt":"2019-01-22T23:05:37Z","messageCount":7,"participants":["Patrick Hogg","Duy Nguyen","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"367043","messageId":"20190118022736.36832-1-phogg@novamoon.net","threadId":"50255","inReplyTo":null,"subject":"[PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-18T02:27:36Z","receivedAt":"2019-01-18T02:28:20Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n2018-04-14) added an extra usage of read_lock/read_unlock in the newly\nintroduced oe_get_size_slow for thread safety in parallel calls to\ntry_delta(). Unfortunately oe_get_size_slow is also used in serial\ncode, some of which is called before the first invocation of\nll_find_deltas. As such the read mutex is not guaranteed to be\ninitialized.\n\nResolve this by splitting off the read mutex initialization from\ninit_threaded_search. Instead initialize (and clean up) the read\nmutex in cmd_pack_objects.\n\nSigned-off-by: Patrick Hogg \n---\n builtin/pack-objects.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 411aefd68..9084bef02 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond;\n */\n static void init_threaded_search(void)\n {\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);\n \told_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);\n }\n \n+static void init_read_mutex(void)\n+{\n+\tinit_recursive_mutex(&read_mutex);\n+}\n+\n static void cleanup_threaded_search(void)\n {\n \tset_try_to_free_routine(old_try_to_free_routine);\n \tpthread_cond_destroy(&progress_cond);\n-\tpthread_mutex_destroy(&read_mutex);\n \tpthread_mutex_destroy(&cache_mutex);\n \tpthread_mutex_destroy(&progress_mutex);\n }\n \n+static void cleanup_read_mutex(void)\n+{\n+\tpthread_mutex_destroy(&read_mutex);\n+}\n+\n static void *threaded_find_deltas(void *arg)\n {\n \tstruct thread_params *me = arg;\n@@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n+\tinit_read_mutex();\n+\n \tif (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))\n \t\tBUG(\"too many dfs states, increase OE_DFS_STATE_BITS\");\n \n@@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t _(\"Total %\"PRIu32\" (delta %\"PRIu32\"),\"\n \t\t\t \" reused %\"PRIu32\" (delta %\"PRIu32\")\"),\n \t\t\t written, written_delta, reused, reused_delta);\n+\n+\tcleanup_read_mutex();\n \treturn 0;\n }\n-- \n2.20.1.windows.1\n\n"},{"id":"367064","messageId":"CACsJy8CRQdzMTZU0Khno+r+kqrzu-iG5J_fKnPfVMWe55azD2g@mail.gmail.com","threadId":"50255","inReplyTo":"20190118022736.36832-1-phogg@novamoon.net","subject":"Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-18T09:21:02Z","receivedAt":"2019-01-18T09:21:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg wrote:\n>\n> ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n> 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n> introduced oe_get_size_slow for thread safety in parallel calls to\n> try_delta(). Unfortunately oe_get_size_slow is also used in serial\n> code, some of which is called before the first invocation of\n> ll_find_deltas. As such the read mutex is not guaranteed to be\n> initialized.\n\nThis must be the SIZE() macros in type_size_sort(), isn't it? I think\nwe hit the same problem (use of uninitialized mutex) in this same code\nnot long ago. I wonder if there's anyway we can reliably test and\ncatch this.\n\n> Resolve this by splitting off the read mutex initialization from\n> init_threaded_search. Instead initialize (and clean up) the read\n> mutex in cmd_pack_objects.\n\nMaybe move the mutex to 'struct packing_data' and initialize it in\nprepare_packing_data(), so we centralize mutex at two locations:\ngeneric ones go there, command-specific mutexes stay here in\ninit_threaded_search(). We could also move oe_get_size_slow() back to\npack-objects.c (the one outside builtin/).\n\n> Signed-off-by: Patrick Hogg \n> ---\n> builtin/pack-objects.c | 16 ++++++++++++++--\n> 1 file changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 411aefd68..9084bef02 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond;\n> */\n> static void init_threaded_search(void)\n> {\n> - init_recursive_mutex(&read_mutex);\n> pthread_mutex_init(&cache_mutex, NULL);\n> pthread_mutex_init(&progress_mutex, NULL);\n> pthread_cond_init(&progress_cond, NULL);\n> old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);\n> }\n>\n> +static void init_read_mutex(void)\n> +{\n> + init_recursive_mutex(&read_mutex);\n> +}\n> +\n> static void cleanup_threaded_search(void)\n> {\n> set_try_to_free_routine(old_try_to_free_routine);\n> pthread_cond_destroy(&progress_cond);\n> - pthread_mutex_destroy(&read_mutex);\n> pthread_mutex_destroy(&cache_mutex);\n> pthread_mutex_destroy(&progress_mutex);\n> }\n>\n> +static void cleanup_read_mutex(void)\n> +{\n> + pthread_mutex_destroy(&read_mutex);\n> +}\n> +\n> static void *threaded_find_deltas(void *arg)\n> {\n> struct thread_params *me = arg;\n> @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> OPT_END(),\n> };\n>\n> + init_read_mutex();\n> +\n> if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))\n> BUG(\"too many dfs states, increase OE_DFS_STATE_BITS\");\n>\n> @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> _(\"Total %\"PRIu32\" (delta %\"PRIu32\"),\"\n> \" reused %\"PRIu32\" (delta %\"PRIu32\")\"),\n> written, written_delta, reused, reused_delta);\n> +\n> + cleanup_read_mutex();\n> return 0;\n> }\n> --\n> 2.20.1.windows.1\n>\n\n\n-- \nDuy\n"},{"id":"367081","messageId":"nycvar.QRO.7.76.6.1901181150450.41@tvgsbejvaqbjf.bet","threadId":"50255","inReplyTo":"20190118022736.36832-1-phogg@novamoon.net","subject":"Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T10:54:54Z","receivedAt":"2019-01-18T10:55:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 17 Jan 2019, Patrick Hogg wrote:\n\n> ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n> 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n> introduced oe_get_size_slow for thread safety in parallel calls to\n> try_delta(). Unfortunately oe_get_size_slow is also used in serial\n> code, some of which is called before the first invocation of\n> ll_find_deltas. As such the read mutex is not guaranteed to be\n> initialized.\n> \n> Resolve this by splitting off the read mutex initialization from\n> init_threaded_search. Instead initialize (and clean up) the read\n> mutex in cmd_pack_objects.\n\nVery good explanation.\n\nTwo suggestions:\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 411aefd68..9084bef02 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond;\n> */\n> static void init_threaded_search(void)\n> {\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);\n> \told_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);\n> }\n> \n> +static void init_read_mutex(void)\n> +{\n> +\tinit_recursive_mutex(&read_mutex);\n> +}\n> +\n> static void cleanup_threaded_search(void)\n> {\n> \tset_try_to_free_routine(old_try_to_free_routine);\n> \tpthread_cond_destroy(&progress_cond);\n> -\tpthread_mutex_destroy(&read_mutex);\n> \tpthread_mutex_destroy(&cache_mutex);\n> \tpthread_mutex_destroy(&progress_mutex);\n> }\n> \n> +static void cleanup_read_mutex(void)\n> +{\n> +\tpthread_mutex_destroy(&read_mutex);\n> +}\n> +\n> static void *threaded_find_deltas(void *arg)\n> {\n> \tstruct thread_params *me = arg;\n> @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\tOPT_END(),\n> \t};\n> \n> +\tinit_read_mutex();\n\nAs the `read_mutex` is file-local, and as it really is only initialized\n(or for that matter, cleaned up) in *one* spot, why not just spell out the\none-liner instead of introducing two new functions?\n\n> +\n> \tif (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))\n> \t\tBUG(\"too many dfs states, increase OE_DFS_STATE_BITS\");\n> \n> @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\t\t _(\"Total %\"PRIu32\" (delta %\"PRIu32\"),\"\n> \t\t\t \" reused %\"PRIu32\" (delta %\"PRIu32\")\"),\n> \t\t\t written, written_delta, reused, reused_delta);\n> +\n> +\tcleanup_read_mutex();\n\nThis misses one early `return`:\n\n\tif (non_empty && !nr_result)\n\t\treturn 0;\n\nI'd still suggest to just write out\n\n\tif (non_empty && !nr_result) {\n\t\tpthread_mutex_destroy(&read_mutex);\n\t\treturn 0;\n\t}\n\neven if there are now two call sites.\n\nCiao,\nJohannes\n\n> \treturn 0;\n> }\n> -- \n> 2.20.1.windows.1\n> \n> \n"},{"id":"367090","messageId":"CAFOcBz=pcLO-t0FSPHQ9eos4SN+nK3kyFr2xTTyGNu447PFnQw@mail.gmail.com","threadId":"50255","inReplyTo":"CAFOcBzmCWBjng_HqFthSrg3eKcEHpQLaa5buKAcm8JHt7EsGdA@mail.gmail.com","subject":"Fwd: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-18T13:07:20Z","receivedAt":"2019-01-18T13:08:03Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"On Fri, Jan 18, 2019 at 4:21 AM Duy Nguyen wrote:\n>\n> On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg wrote:\n> >\n> > ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n> > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n> > introduced oe_get_size_slow for thread safety in parallel calls to\n> > try_delta(). Unfortunately oe_get_size_slow is also used in serial\n> > code, some of which is called before the first invocation of\n> > ll_find_deltas. As such the read mutex is not guaranteed to be\n> > initialized.\n>\n> This must be the SIZE() macros in type_size_sort(), isn't it? I think\n> we hit the same problem (use of uninitialized mutex) in this same code\n> not long ago. I wonder if there's anyway we can reliably test and\n> catch this.\n\nIt was actually the SET_SIZE macro in check_object, at least for the\nrepo at my company that hits this issue. I took a look at the call\ntree for oe_get_size_slow and found that it's used in many places\noutside of ll_find_deltas, so there are many potential call sites\nwhere this could crop up:\noe_get_size_slow\n oe_size (SIZE macro)\n write_reuse_object\n write_object\n write_one\n write_pack_file\n cmd_pack_objects\n type_size_sort\n prepare_pack\n cmd_pack_objects\n try_delta\n find_deltas\n threaded_find_deltas\n ll_find_deltas\n prepare_pack\n cmd_pack_objects\n ll_find_deltas\n prepare_pack\n cmd_pack_objects\n free_unpacked\n find_deltas\n threaded_find_deltas\n ll_find_deltas\n prepare_pack\n cmd_pack_objects\n ll_find_deltas\n prepare_pack\n cmd_pack_objects\n oe_size_less_than\n prepare_pack\n cmd_pack_objects\n oe_size_greater_than\n write_no_reuse_object\n write_reuse_object\n write_object\n write_one\n write_pack_file\n cmd_pack_objects\n write_object\n write_one\n write_pack_file\n cmd_pack_objects\n get_object_details\n prepare_pack\n cmd_pack_objects\n oe_set_size (SET_SIZE macro)\n check_object\n get_object_details\n prepare_pack\n cmd_pack_objects\n drop_reused_delta\n break_delta_chains\n get_object_details\n prepare_pack\n cmd_pack_objects\n(Sorry if this is redundant for those who know the code better)\n\n>\n>\n> > Resolve this by splitting off the read mutex initialization from\n> > init_threaded_search. Instead initialize (and clean up) the read\n> > mutex in cmd_pack_objects.\n>\n> Maybe move the mutex to 'struct packing_data' and initialize it in\n> prepare_packing_data(), so we centralize mutex at two locations:\n> generic ones go there, command-specific mutexes stay here in\n> init_threaded_search(). We could also move oe_get_size_slow() back to\n> pack-objects.c (the one outside builtin/).\n\nI was already thinking that generic mutexes should be separated from\ncommand specific ones (that's why I introduced init_read_mutex and\ncleanup_read_mutex, but that may well not be the right exposure.)\nI'll try my hand at this tonight (just moving the mutex to struct\npacking_data and initializing it in prepare_packing_data, I'll leave\nlarge code moves to the experts) and see how it turns out.\n\n>\n>\n> > Signed-off-by: Patrick Hogg \n> > ---\n> > builtin/pack-objects.c | 16 ++++++++++++++--\n> > 1 file changed, 14 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 411aefd68..9084bef02 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond;\n> > */\n> > static void init_threaded_search(void)\n> > {\n> > - init_recursive_mutex(&read_mutex);\n> > pthread_mutex_init(&cache_mutex, NULL);\n> > pthread_mutex_init(&progress_mutex, NULL);\n> > pthread_cond_init(&progress_cond, NULL);\n> > old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);\n> > }\n> >\n> > +static void init_read_mutex(void)\n> > +{\n> > + init_recursive_mutex(&read_mutex);\n> > +}\n> > +\n> > static void cleanup_threaded_search(void)\n> > {\n> > set_try_to_free_routine(old_try_to_free_routine);\n> > pthread_cond_destroy(&progress_cond);\n> > - pthread_mutex_destroy(&read_mutex);\n> > pthread_mutex_destroy(&cache_mutex);\n> > pthread_mutex_destroy(&progress_mutex);\n> > }\n> >\n> > +static void cleanup_read_mutex(void)\n> > +{\n> > + pthread_mutex_destroy(&read_mutex);\n> > +}\n> > +\n> > static void *threaded_find_deltas(void *arg)\n> > {\n> > struct thread_params *me = arg;\n> > @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> > OPT_END(),\n> > };\n> >\n> > + init_read_mutex();\n> > +\n> > if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))\n> > BUG(\"too many dfs states, increase OE_DFS_STATE_BITS\");\n> >\n> > @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> > _(\"Total %\"PRIu32\" (delta %\"PRIu32\"),\"\n> > \" reused %\"PRIu32\" (delta %\"PRIu32\")\"),\n> > written, written_delta, reused, reused_delta);\n> > +\n> > + cleanup_read_mutex();\n> > return 0;\n> > }\n> > --\n> > 2.20.1.windows.1\n> >\n>\n>\n> --\n> Duy\n"},{"id":"367091","messageId":"CACsJy8A-kk9DEPZKWR26ye5tUHjny_18xf69CUevpRLJrs19PA@mail.gmail.com","threadId":"50255","inReplyTo":"CAFOcBzmCWBjng_HqFthSrg3eKcEHpQLaa5buKAcm8JHt7EsGdA@mail.gmail.com","subject":"Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-18T13:09:59Z","receivedAt":"2019-01-18T13:10:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 18, 2019 at 8:04 PM Patrick Hogg wrote:\n>\n> On Fri, Jan 18, 2019 at 4:21 AM Duy Nguyen wrote:\n>>\n>> On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg wrote:\n>> >\n>> > ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n>> > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n>> > introduced oe_get_size_slow for thread safety in parallel calls to\n>> > try_delta(). Unfortunately oe_get_size_slow is also used in serial\n>> > code, some of which is called before the first invocation of\n>> > ll_find_deltas. As such the read mutex is not guaranteed to be\n>> > initialized.\n>>\n>> This must be the SIZE() macros in type_size_sort(), isn't it? I think\n>> we hit the same problem (use of uninitialized mutex) in this same code\n>> not long ago. I wonder if there's anyway we can reliably test and\n>> catch this.\n>\n>\n> It was actually the SET_SIZE macro in check_object, at least for the repo at my company that hits this issue. I took a look at the call tree for oe_get_size_slow and found that it's used in many places outside of ll_find_deltas, so there are many potential call sites where this could crop up:\n>\n> [snip]\n>\n\nAh, yes. I think the only problematic place is from prepare_pack().\nThe single threaded access after ll_find_deltas() is fine because we\nnever destroy mutexes.\n\n> (Sorry if this is redundant for those who know the code better)\n\nActually it's me to say sorry. I apparently did not know the code flow\ngood enough to prevent this problem in the first place.\n\n>> > Resolve this by splitting off the read mutex initialization from\n>> > init_threaded_search. Instead initialize (and clean up) the read\n>> > mutex in cmd_pack_objects.\n>>\n>> Maybe move the mutex to 'struct packing_data' and initialize it in\n>> prepare_packing_data(), so we centralize mutex at two locations:\n>> generic ones go there, command-specific mutexes stay here in\n>> init_threaded_search(). We could also move oe_get_size_slow() back to\n>> pack-objects.c (the one outside builtin/).\n>\n>\n> I was already thinking that generic mutexes should be separated from command specific ones (that's why I introduced init_read_mutex and cleanup_read_mutex, but that may well not be the right exposure.) I'll try my hand at this tonight (just moving the mutex to struct packing_data and initializing it in prepare_packing_data, I'll leave large code moves to the experts) and see how it turns out.\n\nYes, leave the code move for now. Bug fixes stay small and simple (and\nget merged faster)\n-- \nDuy\n"},{"id":"367168","messageId":"CAFOcBzmPAfq2ihF191KQAu0-_hz8DLdeAwDO-+gA-dR1sdND8Q@mail.gmail.com","threadId":"50255","inReplyTo":"CACsJy8A-kk9DEPZKWR26ye5tUHjny_18xf69CUevpRLJrs19PA@mail.gmail.com","subject":"Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-19T01:46:37Z","receivedAt":"2019-01-19T01:47:21Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"On Fri, Jan 18, 2019 at 8:10 AM Duy Nguyen wrote:\n>\n> On Fri, Jan 18, 2019 at 8:04 PM Patrick Hogg wrote:\n> >\n> > On Fri, Jan 18, 2019 at 4:21 AM Duy Nguyen wrote:\n> >>\n> >> On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg wrote:\n> >> >\n> >> > ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n> >> > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n> >> > introduced oe_get_size_slow for thread safety in parallel calls to\n> >> > try_delta(). Unfortunately oe_get_size_slow is also used in serial\n> >> > code, some of which is called before the first invocation of\n> >> > ll_find_deltas. As such the read mutex is not guaranteed to be\n> >> > initialized.\n> >>\n> >> This must be the SIZE() macros in type_size_sort(), isn't it? I think\n> >> we hit the same problem (use of uninitialized mutex) in this same code\n> >> not long ago. I wonder if there's anyway we can reliably test and\n> >> catch this.\n> >\n> >\n> > It was actually the SET_SIZE macro in check_object, at least for the repo at my company that hits this issue. I took a look at the call tree for oe_get_size_slow and found that it's used in many places outside of ll_find_deltas, so there are many potential call sites where this could crop up:\n> >\n> > [snip]\n> >\n>\n> Ah, yes. I think the only problematic place is from prepare_pack().\n> The single threaded access after ll_find_deltas() is fine because we\n> never destroy mutexes.\n\nI'm a bit confused, I see calls to pthread_mutex_destroy in\ncleanup_threaded_search. It's true that only\nprepare_packing_data(&to_pack) is called and there is no cleanup of\nthe to_pack instance (at least as far as I can see) in\ncmd_pack_objects, but aren't the threaded_search mutexes destroyed?\n\n>\n> > (Sorry if this is redundant for those who know the code better)\n>\n> Actually it's me to say sorry. I apparently did not know the code flow\n> good enough to prevent this problem in the first place.\n>\n> >> > Resolve this by splitting off the read mutex initialization from\n> >> > init_threaded_search. Instead initialize (and clean up) the read\n> >> > mutex in cmd_pack_objects.\n> >>\n> >> Maybe move the mutex to 'struct packing_data' and initialize it in\n> >> prepare_packing_data(), so we centralize mutex at two locations:\n> >> generic ones go there, command-specific mutexes stay here in\n> >> init_threaded_search(). We could also move oe_get_size_slow() back to\n> >> pack-objects.c (the one outside builtin/).\n> >\n> >\n> > I was already thinking that generic mutexes should be separated from command specific ones (that's why I introduced init_read_mutex and cleanup_read_mutex, but that may well not be the right exposure.) I'll try my hand at this tonight (just moving the mutex to struct packing_data and initializing it in prepare_packing_data, I'll leave large code moves to the experts) and see how it turns out.\n>\n> Yes, leave the code move for now. Bug fixes stay small and simple (and\n> get merged faster)\n\nI was looking at this and noticed that packing_data already has a lock\nmutex member. Perhaps I am missing something but would it be\nappropriate to drop read_mutex altogether, change lock to be a\nrecursive mutex, then use that instead in read_lock()/read_unlock()?\n(Or even to directly call packing_data_lock/packing_data_unlock\ninstead of read_lock/read_unlock? Strictly speaking it would be a\npack lock and not a read lock so the read_lock/read_unlock terminology\nwouldn't be accurate anymore.)\n\nI have the change locally to move read_mutex to the packing_data\nstruct (and rename it to read_lock to be consistent with the \"lock\"\nmember), but it seems redundant. (And the lock member is only used in\noe_set_delta_size.)\n\n> --\n> Duy\n"},{"id":"367388","messageId":"xmqqwomwcmyt.fsf@gitster-ct.c.googlers.com","threadId":"50255","inReplyTo":"20190118022736.36832-1-phogg@novamoon.net","subject":"Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T23:05:30Z","receivedAt":"2019-01-22T23:05:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Hogg writes:\n\n> @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\tOPT_END(),\n> \t};\n> \n> +\tinit_read_mutex();\n> +\n> \tif (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))\n> \t\tBUG(\"too many dfs states, increase OE_DFS_STATE_BITS\");\n> \n> @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\t\t _(\"Total %\"PRIu32\" (delta %\"PRIu32\"),\"\n> \t\t\t \" reused %\"PRIu32\" (delta %\"PRIu32\")\"),\n> \t\t\t written, written_delta, reused, reused_delta);\n> +\n> +\tcleanup_read_mutex();\n\nThis is insufficient, isn't it? There is \"return 0\" just above the\npre-context of this hunk which should probably be made to \"goto\nclean_exit\" or something, with a new label \"clean_exit:\" just in\nfront of this new call to cleaup.\n\n> \treturn 0;\n> }\n"}]}