[PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[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 using the existing lock in packing_data which is
initialized early in cmd_pack_objects instead of read_mutex.
Additionally, upgrade the packing_data lock to a recursive mutex to
make it a suitable replacement for read_mutex.

Signed-off-by: Patrick Hogg <phogg@novamoon.net>
---

As I mentioned in the prior thread I think that it will be simpler
to simply use the existing lock in packing_data instead of moving
read_mutex. I can go back to simply moving read_mutex to the
packing_data struct if that that is preferable, though.

I also removed the #ifndef NO_PTHREADS in prepare_packing_data around
the initialization of &pdata->lock since I had to upgrade the lock to
a recursive mutex. As far as I can tell init_recursive_mutex (and
pthread_mutex_init for that matter) have that protection already so it
appears to be redundant.

 builtin/pack-objects.c | 27 ++++++++++++---------------
 pack-objects.c         |  4 +---
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 411aefd68..5439b434c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 }
 
 /* Protect access to object database */
-static pthread_mutex_t read_mutex;
-#define read_lock()		pthread_mutex_lock(&read_mutex)
-#define read_unlock()		pthread_mutex_unlock(&read_mutex)
+#define pack_lock()		packing_data_lock(&to_pack)
+#define pack_unlock()		packing_data_unlock(&to_pack)
 
 /* Protect delta_cache_size */
 static pthread_mutex_t cache_mutex;
@@ -1993,11 +1992,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 	unsigned long used, avail, size;
 
 	if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
-		read_lock();
+		pack_lock();
 		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
 			die(_("unable to get size of %s"),
 			    oid_to_hex(&e->idx.oid));
-		read_unlock();
+		pack_unlock();
 		return size;
 	}
 
@@ -2005,7 +2004,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 	if (!p)
 		BUG("when e->type is a delta, it must belong to a pack");
 
-	read_lock();
+	pack_lock();
 	w_curs = NULL;
 	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
 	used = unpack_object_header_buffer(buf, avail, &type, &size);
@@ -2014,7 +2013,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 		    oid_to_hex(&e->idx.oid));
 
 	unuse_pack(&w_curs);
-	read_unlock();
+	pack_unlock();
 	return size;
 }
 
@@ -2076,9 +2075,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 
 	/* Load data if not already done */
 	if (!trg->data) {
-		read_lock();
+		pack_lock();
 		trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);
-		read_unlock();
+		pack_unlock();
 		if (!trg->data)
 			die(_("object %s cannot be read"),
 			    oid_to_hex(&trg_entry->idx.oid));
@@ -2089,9 +2088,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 		*mem_usage += sz;
 	}
 	if (!src->data) {
-		read_lock();
+		pack_lock();
 		src->data = read_object_file(&src_entry->idx.oid, &type, &sz);
-		read_unlock();
+		pack_unlock();
 		if (!src->data) {
 			if (src_entry->preferred_base) {
 				static int warned = 0;
@@ -2337,9 +2336,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 
 static void try_to_free_from_threads(size_t size)
 {
-	read_lock();
+	pack_lock();
 	release_pack_memory(size);
-	read_unlock();
+	pack_unlock();
 }
 
 static try_to_free_t old_try_to_free_routine;
@@ -2381,7 +2380,6 @@ 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);
@@ -2392,7 +2390,6 @@ 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);
 }
diff --git a/pack-objects.c b/pack-objects.c
index b6cdbb016..6f32a7ba0 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -148,9 +148,7 @@ void prepare_packing_data(struct packing_data *pdata)
 					     1U << OE_SIZE_BITS);
 	pdata->oe_delta_size_limit = git_env_ulong("GIT_TEST_OE_DELTA_SIZE",
 						   1UL << OE_DELTA_SIZE_BITS);
-#ifndef NO_PTHREADS
-	pthread_mutex_init(&pdata->lock, NULL);
-#endif
+	init_recursive_mutex(&pdata->lock);
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
-- 
2.20.1.windows.1

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Duy Nguyen]

On Sat, Jan 19, 2019 at 10:45 PM 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.
>
> Resolve this by using the existing lock in packing_data which is
> initialized early in cmd_pack_objects instead of read_mutex.
> Additionally, upgrade the packing_data lock to a recursive mutex to
> make it a suitable replacement for read_mutex.
>
> Signed-off-by: Patrick Hogg <phogg@novamoon.net>
> ---
>
> As I mentioned in the prior thread I think that it will be simpler
> to simply use the existing lock in packing_data instead of moving
> read_mutex. I can go back to simply moving read_mutex to the
> packing_data struct if that that is preferable, though.

In early iterations of these changes, I think we hit high contention
when sharing the mutex [1]. I don't know if we will hit the same
performance problem again with this patch. It would be great if Elijah
with his zillion core machine could test this out. Otherwise it may be
just safer to keep the two mutexes separate.

[1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/

>
> I also removed the #ifndef NO_PTHREADS in prepare_packing_data around
> the initialization of &pdata->lock since I had to upgrade the lock to
> a recursive mutex. As far as I can tell init_recursive_mutex (and
> pthread_mutex_init for that matter) have that protection already so it
> appears to be redundant.
>
>  builtin/pack-objects.c | 27 ++++++++++++---------------
>  pack-objects.c         |  4 +---
>  2 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 411aefd68..5439b434c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
>  }
>
>  /* Protect access to object database */
> -static pthread_mutex_t read_mutex;
> -#define read_lock()            pthread_mutex_lock(&read_mutex)
> -#define read_unlock()          pthread_mutex_unlock(&read_mutex)
> +#define pack_lock()            packing_data_lock(&to_pack)
> +#define pack_unlock()          packing_data_unlock(&to_pack)
>
>  /* Protect delta_cache_size */
>  static pthread_mutex_t cache_mutex;
> @@ -1993,11 +1992,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
>         unsigned long used, avail, size;
>
>         if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
> -               read_lock();
> +               pack_lock();
>                 if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
>                         die(_("unable to get size of %s"),
>                             oid_to_hex(&e->idx.oid));
> -               read_unlock();
> +               pack_unlock();
>                 return size;
>         }
>
> @@ -2005,7 +2004,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
>         if (!p)
>                 BUG("when e->type is a delta, it must belong to a pack");
>
> -       read_lock();
> +       pack_lock();
>         w_curs = NULL;
>         buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
>         used = unpack_object_header_buffer(buf, avail, &type, &size);
> @@ -2014,7 +2013,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
>                     oid_to_hex(&e->idx.oid));
>
>         unuse_pack(&w_curs);
> -       read_unlock();
> +       pack_unlock();
>         return size;
>  }
>
> @@ -2076,9 +2075,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
>
>         /* Load data if not already done */
>         if (!trg->data) {
> -               read_lock();
> +               pack_lock();
>                 trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);
> -               read_unlock();
> +               pack_unlock();
>                 if (!trg->data)
>                         die(_("object %s cannot be read"),
>                             oid_to_hex(&trg_entry->idx.oid));
> @@ -2089,9 +2088,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
>                 *mem_usage += sz;
>         }
>         if (!src->data) {
> -               read_lock();
> +               pack_lock();
>                 src->data = read_object_file(&src_entry->idx.oid, &type, &sz);
> -               read_unlock();
> +               pack_unlock();
>                 if (!src->data) {
>                         if (src_entry->preferred_base) {
>                                 static int warned = 0;
> @@ -2337,9 +2336,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
>
>  static void try_to_free_from_threads(size_t size)
>  {
> -       read_lock();
> +       pack_lock();
>         release_pack_memory(size);
> -       read_unlock();
> +       pack_unlock();
>  }
>
>  static try_to_free_t old_try_to_free_routine;
> @@ -2381,7 +2380,6 @@ 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);
> @@ -2392,7 +2390,6 @@ 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);
>  }
> diff --git a/pack-objects.c b/pack-objects.c
> index b6cdbb016..6f32a7ba0 100644
> --- a/pack-objects.c
> +++ b/pack-objects.c
> @@ -148,9 +148,7 @@ void prepare_packing_data(struct packing_data *pdata)
>                                              1U << OE_SIZE_BITS);
>         pdata->oe_delta_size_limit = git_env_ulong("GIT_TEST_OE_DELTA_SIZE",
>                                                    1UL << OE_DELTA_SIZE_BITS);
> -#ifndef NO_PTHREADS
> -       pthread_mutex_init(&pdata->lock, NULL);
> -#endif
> +       init_recursive_mutex(&pdata->lock);
>  }
>
>  struct object_entry *packlist_alloc(struct packing_data *pdata,
> --
> 2.20.1.windows.1
>


-- 
Duy

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Jeff King]

On Mon, Jan 21, 2019 at 05:02:33PM +0700, Duy Nguyen wrote:

> > As I mentioned in the prior thread I think that it will be simpler
> > to simply use the existing lock in packing_data instead of moving
> > read_mutex. I can go back to simply moving read_mutex to the
> > packing_data struct if that that is preferable, though.
> 
> In early iterations of these changes, I think we hit high contention
> when sharing the mutex [1]. I don't know if we will hit the same
> performance problem again with this patch. It would be great if Elijah
> with his zillion core machine could test this out. Otherwise it may be
> just safer to keep the two mutexes separate.
> 
> [1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/

I haven't been following this thread closely, but I still have access to
a 40-core machine if you'd like me to time anything.

It sounds like _this_ patch is the more fine-grained one. Is the more
coarse-grained one already written?

-Peff

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Duy Nguyen]

On Tue, Jan 22, 2019 at 2:28 PM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jan 21, 2019 at 05:02:33PM +0700, Duy Nguyen wrote:
>
> > > As I mentioned in the prior thread I think that it will be simpler
> > > to simply use the existing lock in packing_data instead of moving
> > > read_mutex. I can go back to simply moving read_mutex to the
> > > packing_data struct if that that is preferable, though.
> >
> > In early iterations of these changes, I think we hit high contention
> > when sharing the mutex [1]. I don't know if we will hit the same
> > performance problem again with this patch. It would be great if Elijah
> > with his zillion core machine could test this out. Otherwise it may be
> > just safer to keep the two mutexes separate.
> >
> > [1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/
>
> I haven't been following this thread closely, but I still have access to
> a 40-core machine if you'd like me to time anything.
>
> It sounds like _this_ patch is the more fine-grained one. Is the more
> coarse-grained one already written?

A more fine-grained one would be 'master' where we use two separate
mutexes for different code. I guess if repack performance with this
patch is still the same as 'master', we're good to go. You may need to
lower $GIT_TEST_OE_SIZE and $GIT_TEST_OE_DELTA_SIZE to force more lock
contention.
-- 
Duy

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Patrick Hogg]

On Tue, Jan 22, 2019 at 5:26 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Tue, Jan 22, 2019 at 2:28 PM Jeff King <peff@peff.net> wrote:
> >
> > On Mon, Jan 21, 2019 at 05:02:33PM +0700, Duy Nguyen wrote:
> >
> > > > As I mentioned in the prior thread I think that it will be simpler
> > > > to simply use the existing lock in packing_data instead of moving
> > > > read_mutex. I can go back to simply moving read_mutex to the
> > > > packing_data struct if that that is preferable, though.
> > >
> > > In early iterations of these changes, I think we hit high contention
> > > when sharing the mutex [1]. I don't know if we will hit the same
> > > performance problem again with this patch. It would be great if Elijah
> > > with his zillion core machine could test this out. Otherwise it may be
> > > just safer to keep the two mutexes separate.
> > >
> > > [1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/
> >
> > I haven't been following this thread closely, but I still have access to
> > a 40-core machine if you'd like me to time anything.
> >
> > It sounds like _this_ patch is the more fine-grained one. Is the more
> > coarse-grained one already written?
>
> A more fine-grained one would be 'master' where we use two separate
> mutexes for different code. I guess if repack performance with this
> patch is still the same as 'master', we're good to go. You may need to
> lower $GIT_TEST_OE_SIZE and $GIT_TEST_OE_DELTA_SIZE to force more lock
> contention.

I do have a patch prepared which simply moves read_mutex to the
packing_data struct instead (and renames it read_lock for consistency
with the exiting mutex named "lock"), but I wanted to wait for the
testing regarding lock contention first. I'm prepared either way it
goes.

-Patrick

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Elijah Newren]

On Mon, Jan 21, 2019 at 2:02 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Sat, Jan 19, 2019 at 10:45 PM 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.
> >
> > Resolve this by using the existing lock in packing_data which is
> > initialized early in cmd_pack_objects instead of read_mutex.
> > Additionally, upgrade the packing_data lock to a recursive mutex to
> > make it a suitable replacement for read_mutex.
> >
> > Signed-off-by: Patrick Hogg <phogg@novamoon.net>
> > ---
> >
> > As I mentioned in the prior thread I think that it will be simpler
> > to simply use the existing lock in packing_data instead of moving
> > read_mutex. I can go back to simply moving read_mutex to the
> > packing_data struct if that that is preferable, though.
>
> In early iterations of these changes, I think we hit high contention
> when sharing the mutex [1]. I don't know if we will hit the same
> performance problem again with this patch. It would be great if Elijah
> with his zillion core machine could test this out. Otherwise it may be
> just safer to keep the two mutexes separate.

Testing...

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Elijah Newren]

Duy, Patrick,

On Tue, Jan 22, 2019 at 9:52 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Jan 21, 2019 at 2:02 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Sat, Jan 19, 2019 at 10:45 PM 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.
> > >
> > > Resolve this by using the existing lock in packing_data which is
> > > initialized early in cmd_pack_objects instead of read_mutex.
> > > Additionally, upgrade the packing_data lock to a recursive mutex to
> > > make it a suitable replacement for read_mutex.
> > >
> > > Signed-off-by: Patrick Hogg <phogg@novamoon.net>
> > > ---
> > >
> > > As I mentioned in the prior thread I think that it will be simpler
> > > to simply use the existing lock in packing_data instead of moving
> > > read_mutex. I can go back to simply moving read_mutex to the
> > > packing_data struct if that that is preferable, though.
> >
> > In early iterations of these changes, I think we hit high contention
> > when sharing the mutex [1]. I don't know if we will hit the same
> > performance problem again with this patch. It would be great if Elijah
> > with his zillion core machine could test this out. Otherwise it may be
> > just safer to keep the two mutexes separate.
>

Before this patch, repacking an old clone of linux.git on a 40-core
box (an m4.10xlarge) using the command
   /usr/bin/time -f 'MaxRSS:%M Time:%e' git gc --aggressive
based on commit 16a465bc01 ("Third batch after 2.20", 2019-01-18), resulted in:

MaxRSS:10959072 Time:650.63

Since that was a cold cache, might have had loose objects and what
not, I threw that one out and ran three more times:

MaxRSS: 9886284 Time:571.40
MaxRSS:10441716 Time:566.47
MaxRSS:10157536 Time:569.79

Then I applied this patch and ran three more times:

MaxRSS:10611444 Time:574.60
MaxRSS:10429732 Time:571.48
MaxRSS:10657160 Time:575.58

(Yeah, we talked about MaxRSS not being the most useful last time, but
I'm just copying the command I used last time for consistency; I'm
only really paying attention to Time.)

So, it's within 1% of the timing of the current version.  Seems fine to me.


Hope that helps,
Elijah

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Junio C Hamano]

Patrick Hogg <phogg@novamoon.net> writes:

> As I mentioned in the prior thread I think that it will be simpler
> to simply use the existing lock in packing_data instead of moving
> read_mutex. I can go back to simply moving read_mutex to the
> packing_data struct if that that is preferable, though.

I'll let others comment on this to show preference between the two
approaches.

> I also removed the #ifndef NO_PTHREADS in prepare_packing_data around
> the initialization of &pdata->lock since I had to upgrade the lock to
> a recursive mutex. As far as I can tell init_recursive_mutex (and
> pthread_mutex_init for that matter) have that protection already so it
> appears to be redundant.

If you can defer "I also" to a separate patch, please do so.
Keeping the fix alone as small as possible and not tangled with
other changes would make it easier for people to cherry-pick the fix
to older maintenance tracks if they choose to.

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Patrick Hogg]

On Tue, Jan 22, 2019 at 5:43 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Hogg <phogg@novamoon.net> writes:
>
> > As I mentioned in the prior thread I think that it will be simpler
> > to simply use the existing lock in packing_data instead of moving
> > read_mutex. I can go back to simply moving read_mutex to the
> > packing_data struct if that that is preferable, though.
>
> I'll let others comment on this to show preference between the two
> approaches.
>
> > I also removed the #ifndef NO_PTHREADS in prepare_packing_data around
> > the initialization of &pdata->lock since I had to upgrade the lock to
> > a recursive mutex. As far as I can tell init_recursive_mutex (and
> > pthread_mutex_init for that matter) have that protection already so it
> > appears to be redundant.
>
> If you can defer "I also" to a separate patch, please do so.
> Keeping the fix alone as small as possible and not tangled with
> other changes would make it easier for people to cherry-pick the fix
> to older maintenance tracks if they choose to.

That's a fair point.  To confirm (as I'm rather new to submitting git
patches), do you mean to submit a two-patch series or to just leave
out the #ifndef removal altogether for now?

If this does become a two patch series I could simply move the
read_mutex to packing_data in the first patch and merge the two
mutexes (and remove the #ifndef) in the second.  That would keep the
fix alone even smaller (just the first patch) to simplify
cherry-picking.

(There is also the option of going back to the v1 change and
correcting the cleanup in the early return.)

I just want to confirm preferences first before submitting a v3 to
avoid spamming patches; I'll go whichever way the experts think is
best.

Thanks,
-Patrick

Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex

[Junio C Hamano]

Patrick Hogg <phogg@novamoon.net> writes:

> On Tue, Jan 22, 2019 at 5:43 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Patrick Hogg <phogg@novamoon.net> writes:
>>
>> > As I mentioned in the prior thread I think that it will be simpler
>> > to simply use the existing lock in packing_data instead of moving
>> > read_mutex. I can go back to simply moving read_mutex to the
>> > packing_data struct if that that is preferable, though.
>>
>> I'll let others comment on this to show preference between the two
>> approaches.
>>
>> > I also removed the #ifndef NO_PTHREADS in prepare_packing_data around
>> > the initialization of &pdata->lock since I had to upgrade the lock to
>> > a recursive mutex. As far as I can tell init_recursive_mutex (and
>> > pthread_mutex_init for that matter) have that protection already so it
>> > appears to be redundant.
>>
>> If you can defer "I also" to a separate patch, please do so.
>> Keeping the fix alone as small as possible and not tangled with
>> other changes would make it easier for people to cherry-pick the fix
>> to older maintenance tracks if they choose to.
>
> That's a fair point.  To confirm (as I'm rather new to submitting git
> patches), do you mean to submit a two-patch series or to just leave
> out the #ifndef removal altogether for now?

Either would work ;-)

> If this does become a two patch series I could simply move the
> read_mutex to packing_data in the first patch and merge the two
> mutexes (and remove the #ifndef) in the second.  That would keep the
> fix alone even smaller (just the first patch) to simplify
> cherry-picking.
>
> (There is also the option of going back to the v1 change and
> correcting the cleanup in the early return.)

Yes, but as I said, I'll let others show their preferences between
approaches v1/v2.