[PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[Patrick Hogg]

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(-)

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

Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[Duy Nguyen]

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
>


-- 
Duy

Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[Johannes Schindelin]

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
> 
> 

Fwd: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[Patrick Hogg]

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

Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[Duy Nguyen]

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)
-- 
Duy

Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[Patrick Hogg]

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

Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

[Junio C Hamano]

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;
>  }