| Message ID | 20190118022736.36832-1-phogg@novamoon.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pack-objects.c: Initialize read mutex in cmd_pack_objects | expand |
On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg <phogg@novamoon.net> wrote: > > ac77d0c37 ("pack-objects: shrink size field in struct object_entry", > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly > introduced oe_get_size_slow for thread safety in parallel calls to > try_delta(). Unfortunately oe_get_size_slow is also used in serial > code, some of which is called before the first invocation of > ll_find_deltas. As such the read mutex is not guaranteed to be > initialized. This must be the SIZE() macros in type_size_sort(), isn't it? I think we hit the same problem (use of uninitialized mutex) in this same code not long ago. I wonder if there's anyway we can reliably test and catch this. > Resolve this by splitting off the read mutex initialization from > init_threaded_search. Instead initialize (and clean up) the read > mutex in cmd_pack_objects. Maybe move the mutex to 'struct packing_data' and initialize it in prepare_packing_data(), so we centralize mutex at two locations: generic ones go there, command-specific mutexes stay here in init_threaded_search(). We could also move oe_get_size_slow() back to pack-objects.c (the one outside builtin/). > Signed-off-by: Patrick Hogg <phogg@novamoon.net> > --- > builtin/pack-objects.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 411aefd68..9084bef02 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond; > */ > static void init_threaded_search(void) > { > - init_recursive_mutex(&read_mutex); > pthread_mutex_init(&cache_mutex, NULL); > pthread_mutex_init(&progress_mutex, NULL); > pthread_cond_init(&progress_cond, NULL); > old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads); > } > > +static void init_read_mutex(void) > +{ > + init_recursive_mutex(&read_mutex); > +} > + > static void cleanup_threaded_search(void) > { > set_try_to_free_routine(old_try_to_free_routine); > pthread_cond_destroy(&progress_cond); > - pthread_mutex_destroy(&read_mutex); > pthread_mutex_destroy(&cache_mutex); > pthread_mutex_destroy(&progress_mutex); > } > > +static void cleanup_read_mutex(void) > +{ > + pthread_mutex_destroy(&read_mutex); > +} > + > static void *threaded_find_deltas(void *arg) > { > struct thread_params *me = arg; > @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > OPT_END(), > }; > > + init_read_mutex(); > + > if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS)) > BUG("too many dfs states, increase OE_DFS_STATE_BITS"); > > @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > _("Total %"PRIu32" (delta %"PRIu32")," > " reused %"PRIu32" (delta %"PRIu32")"), > written, written_delta, reused, reused_delta); > + > + cleanup_read_mutex(); > return 0; > } > -- > 2.20.1.windows.1 >
Hi Patrick, On Thu, 17 Jan 2019, Patrick Hogg wrote: > ac77d0c37 ("pack-objects: shrink size field in struct object_entry", > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly > introduced oe_get_size_slow for thread safety in parallel calls to > try_delta(). Unfortunately oe_get_size_slow is also used in serial > code, some of which is called before the first invocation of > ll_find_deltas. As such the read mutex is not guaranteed to be > initialized. > > Resolve this by splitting off the read mutex initialization from > init_threaded_search. Instead initialize (and clean up) the read > mutex in cmd_pack_objects. Very good explanation. Two suggestions: > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 411aefd68..9084bef02 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond; > */ > static void init_threaded_search(void) > { > - init_recursive_mutex(&read_mutex); > pthread_mutex_init(&cache_mutex, NULL); > pthread_mutex_init(&progress_mutex, NULL); > pthread_cond_init(&progress_cond, NULL); > old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads); > } > > +static void init_read_mutex(void) > +{ > + init_recursive_mutex(&read_mutex); > +} > + > static void cleanup_threaded_search(void) > { > set_try_to_free_routine(old_try_to_free_routine); > pthread_cond_destroy(&progress_cond); > - pthread_mutex_destroy(&read_mutex); > pthread_mutex_destroy(&cache_mutex); > pthread_mutex_destroy(&progress_mutex); > } > > +static void cleanup_read_mutex(void) > +{ > + pthread_mutex_destroy(&read_mutex); > +} > + > static void *threaded_find_deltas(void *arg) > { > struct thread_params *me = arg; > @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > OPT_END(), > }; > > + init_read_mutex(); As the `read_mutex` is file-local, and as it really is only initialized (or for that matter, cleaned up) in *one* spot, why not just spell out the one-liner instead of introducing two new functions? > + > if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS)) > BUG("too many dfs states, increase OE_DFS_STATE_BITS"); > > @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > _("Total %"PRIu32" (delta %"PRIu32")," > " reused %"PRIu32" (delta %"PRIu32")"), > written, written_delta, reused, reused_delta); > + > + cleanup_read_mutex(); This misses one early `return`: if (non_empty && !nr_result) return 0; I'd still suggest to just write out if (non_empty && !nr_result) { pthread_mutex_destroy(&read_mutex); return 0; } even if there are now two call sites. Ciao, Johannes > return 0; > } > -- > 2.20.1.windows.1 > >
On Fri, Jan 18, 2019 at 4:21 AM Duy Nguyen <pclouds@gmail.com> wrote: > > On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg <phogg@novamoon.net> wrote: > > > > ac77d0c37 ("pack-objects: shrink size field in struct object_entry", > > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly > > introduced oe_get_size_slow for thread safety in parallel calls to > > try_delta(). Unfortunately oe_get_size_slow is also used in serial > > code, some of which is called before the first invocation of > > ll_find_deltas. As such the read mutex is not guaranteed to be > > initialized. > > This must be the SIZE() macros in type_size_sort(), isn't it? I think > we hit the same problem (use of uninitialized mutex) in this same code > not long ago. I wonder if there's anyway we can reliably test and > catch this. 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: oe_get_size_slow oe_size (SIZE macro) write_reuse_object write_object write_one write_pack_file cmd_pack_objects type_size_sort prepare_pack cmd_pack_objects try_delta find_deltas threaded_find_deltas ll_find_deltas prepare_pack cmd_pack_objects ll_find_deltas prepare_pack cmd_pack_objects free_unpacked find_deltas threaded_find_deltas ll_find_deltas prepare_pack cmd_pack_objects ll_find_deltas prepare_pack cmd_pack_objects oe_size_less_than prepare_pack cmd_pack_objects oe_size_greater_than write_no_reuse_object write_reuse_object write_object write_one write_pack_file cmd_pack_objects write_object write_one write_pack_file cmd_pack_objects get_object_details prepare_pack cmd_pack_objects oe_set_size (SET_SIZE macro) check_object get_object_details prepare_pack cmd_pack_objects drop_reused_delta break_delta_chains get_object_details prepare_pack cmd_pack_objects (Sorry if this is redundant for those who know the code better) > > > > Resolve this by splitting off the read mutex initialization from > > init_threaded_search. Instead initialize (and clean up) the read > > mutex in cmd_pack_objects. > > Maybe move the mutex to 'struct packing_data' and initialize it in > prepare_packing_data(), so we centralize mutex at two locations: > generic ones go there, command-specific mutexes stay here in > init_threaded_search(). We could also move oe_get_size_slow() back to > pack-objects.c (the one outside builtin/). 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. > > > > Signed-off-by: Patrick Hogg <phogg@novamoon.net> > > --- > > builtin/pack-objects.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > > index 411aefd68..9084bef02 100644 > > --- a/builtin/pack-objects.c > > +++ b/builtin/pack-objects.c > > @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond; > > */ > > static void init_threaded_search(void) > > { > > - init_recursive_mutex(&read_mutex); > > pthread_mutex_init(&cache_mutex, NULL); > > pthread_mutex_init(&progress_mutex, NULL); > > pthread_cond_init(&progress_cond, NULL); > > old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads); > > } > > > > +static void init_read_mutex(void) > > +{ > > + init_recursive_mutex(&read_mutex); > > +} > > + > > static void cleanup_threaded_search(void) > > { > > set_try_to_free_routine(old_try_to_free_routine); > > pthread_cond_destroy(&progress_cond); > > - pthread_mutex_destroy(&read_mutex); > > pthread_mutex_destroy(&cache_mutex); > > pthread_mutex_destroy(&progress_mutex); > > } > > > > +static void cleanup_read_mutex(void) > > +{ > > + pthread_mutex_destroy(&read_mutex); > > +} > > + > > static void *threaded_find_deltas(void *arg) > > { > > struct thread_params *me = arg; > > @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > > OPT_END(), > > }; > > > > + init_read_mutex(); > > + > > if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS)) > > BUG("too many dfs states, increase OE_DFS_STATE_BITS"); > > > > @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > > _("Total %"PRIu32" (delta %"PRIu32")," > > " reused %"PRIu32" (delta %"PRIu32")"), > > written, written_delta, reused, reused_delta); > > + > > + cleanup_read_mutex(); > > return 0; > > } > > -- > > 2.20.1.windows.1 > > > > > -- > Duy
On Fri, Jan 18, 2019 at 8:04 PM Patrick Hogg <phogg@novamoon.net> wrote: > > On Fri, Jan 18, 2019 at 4:21 AM Duy Nguyen <pclouds@gmail.com> wrote: >> >> On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg <phogg@novamoon.net> wrote: >> > >> > ac77d0c37 ("pack-objects: shrink size field in struct object_entry", >> > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly >> > introduced oe_get_size_slow for thread safety in parallel calls to >> > try_delta(). Unfortunately oe_get_size_slow is also used in serial >> > code, some of which is called before the first invocation of >> > ll_find_deltas. As such the read mutex is not guaranteed to be >> > initialized. >> >> This must be the SIZE() macros in type_size_sort(), isn't it? I think >> we hit the same problem (use of uninitialized mutex) in this same code >> not long ago. I wonder if there's anyway we can reliably test and >> catch this. > > > 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: > > [snip] > Ah, yes. I think the only problematic place is from prepare_pack(). The single threaded access after ll_find_deltas() is fine because we never destroy mutexes. > (Sorry if this is redundant for those who know the code better) Actually it's me to say sorry. I apparently did not know the code flow good enough to prevent this problem in the first place. >> > Resolve this by splitting off the read mutex initialization from >> > init_threaded_search. Instead initialize (and clean up) the read >> > mutex in cmd_pack_objects. >> >> Maybe move the mutex to 'struct packing_data' and initialize it in >> prepare_packing_data(), so we centralize mutex at two locations: >> generic ones go there, command-specific mutexes stay here in >> init_threaded_search(). We could also move oe_get_size_slow() back to >> pack-objects.c (the one outside builtin/). > > > 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. Yes, leave the code move for now. Bug fixes stay small and simple (and get merged faster)
On Fri, Jan 18, 2019 at 8:10 AM Duy Nguyen <pclouds@gmail.com> wrote: > > On Fri, Jan 18, 2019 at 8:04 PM Patrick Hogg <phogg@novamoon.net> wrote: > > > > On Fri, Jan 18, 2019 at 4:21 AM Duy Nguyen <pclouds@gmail.com> wrote: > >> > >> On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg <phogg@novamoon.net> wrote: > >> > > >> > ac77d0c37 ("pack-objects: shrink size field in struct object_entry", > >> > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly > >> > introduced oe_get_size_slow for thread safety in parallel calls to > >> > try_delta(). Unfortunately oe_get_size_slow is also used in serial > >> > code, some of which is called before the first invocation of > >> > ll_find_deltas. As such the read mutex is not guaranteed to be > >> > initialized. > >> > >> This must be the SIZE() macros in type_size_sort(), isn't it? I think > >> we hit the same problem (use of uninitialized mutex) in this same code > >> not long ago. I wonder if there's anyway we can reliably test and > >> catch this. > > > > > > 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: > > > > [snip] > > > > Ah, yes. I think the only problematic place is from prepare_pack(). > The single threaded access after ll_find_deltas() is fine because we > never destroy mutexes. I'm a bit confused, I see calls to pthread_mutex_destroy in cleanup_threaded_search. It's true that only prepare_packing_data(&to_pack) is called and there is no cleanup of the to_pack instance (at least as far as I can see) in cmd_pack_objects, but aren't the threaded_search mutexes destroyed? > > > (Sorry if this is redundant for those who know the code better) > > Actually it's me to say sorry. I apparently did not know the code flow > good enough to prevent this problem in the first place. > > >> > Resolve this by splitting off the read mutex initialization from > >> > init_threaded_search. Instead initialize (and clean up) the read > >> > mutex in cmd_pack_objects. > >> > >> Maybe move the mutex to 'struct packing_data' and initialize it in > >> prepare_packing_data(), so we centralize mutex at two locations: > >> generic ones go there, command-specific mutexes stay here in > >> init_threaded_search(). We could also move oe_get_size_slow() back to > >> pack-objects.c (the one outside builtin/). > > > > > > 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. > > Yes, leave the code move for now. Bug fixes stay small and simple (and > get merged faster) I was looking at this and noticed that packing_data already has a lock mutex member. Perhaps I am missing something but would it be appropriate to drop read_mutex altogether, change lock to be a recursive mutex, then use that instead in read_lock()/read_unlock()? (Or even to directly call packing_data_lock/packing_data_unlock instead of read_lock/read_unlock? Strictly speaking it would be a pack lock and not a read lock so the read_lock/read_unlock terminology wouldn't be accurate anymore.) I have the change locally to move read_mutex to the packing_data struct (and rename it to read_lock to be consistent with the "lock" member), but it seems redundant. (And the lock member is only used in oe_set_delta_size.) > -- > Duy
Patrick Hogg <phogg@novamoon.net> writes: > @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > OPT_END(), > }; > > + init_read_mutex(); > + > if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS)) > BUG("too many dfs states, increase OE_DFS_STATE_BITS"); > > @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > _("Total %"PRIu32" (delta %"PRIu32")," > " reused %"PRIu32" (delta %"PRIu32")"), > written, written_delta, reused, reused_delta); > + > + cleanup_read_mutex(); This is insufficient, isn't it? There is "return 0" just above the pre-context of this hunk which should probably be made to "goto clean_exit" or something, with a new label "clean_exit:" just in front of this new call to cleaup. > return 0; > }
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 411aefd68..9084bef02 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond; */ static void init_threaded_search(void) { - init_recursive_mutex(&read_mutex); pthread_mutex_init(&cache_mutex, NULL); pthread_mutex_init(&progress_mutex, NULL); pthread_cond_init(&progress_cond, NULL); old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads); } +static void init_read_mutex(void) +{ + init_recursive_mutex(&read_mutex); +} + static void cleanup_threaded_search(void) { set_try_to_free_routine(old_try_to_free_routine); pthread_cond_destroy(&progress_cond); - pthread_mutex_destroy(&read_mutex); pthread_mutex_destroy(&cache_mutex); pthread_mutex_destroy(&progress_mutex); } +static void cleanup_read_mutex(void) +{ + pthread_mutex_destroy(&read_mutex); +} + static void *threaded_find_deltas(void *arg) { struct thread_params *me = arg; @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) OPT_END(), }; + init_read_mutex(); + if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS)) BUG("too many dfs states, increase OE_DFS_STATE_BITS"); @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) _("Total %"PRIu32" (delta %"PRIu32")," " reused %"PRIu32" (delta %"PRIu32")"), written, written_delta, reused, reused_delta); + + cleanup_read_mutex(); return 0; }
ac77d0c37 ("pack-objects: shrink size field in struct object_entry", 2018-04-14) added an extra usage of read_lock/read_unlock in the newly introduced oe_get_size_slow for thread safety in parallel calls to try_delta(). Unfortunately oe_get_size_slow is also used in serial code, some of which is called before the first invocation of ll_find_deltas. As such the read mutex is not guaranteed to be initialized. Resolve this by splitting off the read mutex initialization from init_threaded_search. Instead initialize (and clean up) the read mutex in cmd_pack_objects. Signed-off-by: Patrick Hogg <phogg@novamoon.net> --- builtin/pack-objects.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)