threads / patch / 50303

v3, 2 partspack-objects: move read mutex to packing_data struct

Subject: [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct

## tl;dr

6 messages between Jan 24, 2019 and Jan 25, 2019. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Patrick Hogg· Jan 24, 2019, 01:05 UTC · lore

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 moving the read mutex to packing_data and initializing it in prepare_packing_data which is initialized in cmd_pack_objects.

Signed-off-by: Patrick Hogg <phogg@novamoon.net>
---
 builtin/pack-objects.c |  7 ++-----
 pack-objects.c         |  1 +
 pack-objects.h         | 10 ++++++++++
 3 files changed, 13 insertions(+), 5 deletions(-)
Show changes to 3 files +13 −5

builtin/pack-objects.c, pack-objects.c, pack-objects.h

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 411aefd68..506061b4c 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 read_lock()		packing_data_read_lock(&to_pack)
+#define read_unlock()		packing_data_read_unlock(&to_pack)
 
 /* Protect delta_cache_size */
 static pthread_mutex_t cache_mutex;
@@ -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..3554c43ac 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -150,6 +150,7 @@ void prepare_packing_data(struct packing_data *pdata)
 						   1UL << OE_DELTA_SIZE_BITS);
 #ifndef NO_PTHREADS
 	pthread_mutex_init(&pdata->lock, NULL);
+	init_recursive_mutex(&pdata->read_lock);
 #endif
 }
 
diff --git a/pack-objects.h b/pack-objects.h
index dc869f26c..0a038e3bc 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -146,6 +146,7 @@ struct packing_data {
 	struct packed_git **in_pack;
 
 	pthread_mutex_t lock;
+	pthread_mutex_t read_lock;
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -174,6 +175,15 @@ static inline void packing_data_unlock(struct packing_data *pdata)
 	pthread_mutex_unlock(&pdata->lock);
 }
 
+static inline void packing_data_read_lock(struct packing_data *pdata)
+{
+	pthread_mutex_lock(&pdata->read_lock);
+}
+static inline void packing_data_read_unlock(struct packing_data *pdata)
+{
+	pthread_mutex_unlock(&pdata->read_lock);
+}
+
 struct object_entry *packlist_alloc(struct packing_data *pdata,
 				    const unsigned char *sha1,
 				    uint32_t index_pos);

-- 
2.20.1.windows.1
Patrick Hogg· Jan 24, 2019, 01:05 UTC · re: Patrick Hogg · lore

[PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct

Upgrade the packing_data lock to a recursive mutex to make it suitable for current read_lock usages. Additionally remove the superfluous #ifndef NO_PTHREADS guard around mutex initialization in prepare_packing_data as the mutex functions themselves are already protected.

Signed-off-by: Patrick Hogg <phogg@novamoon.net>
---
 builtin/pack-objects.c | 24 ++++++++++++------------
 pack-objects.c         |  5 +----
 pack-objects.h         | 10 ----------
 3 files changed, 13 insertions(+), 26 deletions(-)
Show changes to 3 files +13 −26

builtin/pack-objects.c, pack-objects.c, pack-objects.h

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 506061b4c..5439b434c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1954,8 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 }
 
 /* Protect access to object database */
-#define read_lock()		packing_data_read_lock(&to_pack)
-#define read_unlock()		packing_data_read_unlock(&to_pack)
+#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;
@@ -1992,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;
 	}
 
@@ -2004,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);
@@ -2013,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;
 }
 
@@ -2075,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));
@@ -2088,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;
@@ -2336,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;
diff --git a/pack-objects.c b/pack-objects.c
index 3554c43ac..6f32a7ba0 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -148,10 +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);
-	init_recursive_mutex(&pdata->read_lock);
-#endif
+	init_recursive_mutex(&pdata->lock);
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
diff --git a/pack-objects.h b/pack-objects.h
index 0a038e3bc..dc869f26c 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -146,7 +146,6 @@ struct packing_data {
 	struct packed_git **in_pack;
 
 	pthread_mutex_t lock;
-	pthread_mutex_t read_lock;
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -175,15 +174,6 @@ static inline void packing_data_unlock(struct packing_data *pdata)
 	pthread_mutex_unlock(&pdata->lock);
 }
 
-static inline void packing_data_read_lock(struct packing_data *pdata)
-{
-	pthread_mutex_lock(&pdata->read_lock);
-}
-static inline void packing_data_read_unlock(struct packing_data *pdata)
-{
-	pthread_mutex_unlock(&pdata->read_lock);
-}
-
 struct object_entry *packlist_alloc(struct packing_data *pdata,
 				    const unsigned char *sha1,
 				    uint32_t index_pos);

-- 
2.20.1.windows.1
Duy Nguyen· Jan 24, 2019, 08:33 UTC · re: Patrick Hogg · lore

Re: [PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct

On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
Show 9 quoted lines
> diff --git a/pack-objects.h b/pack-objects.h
> index 0a038e3bc..dc869f26c 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -146,7 +146,6 @@ struct packing_data {
>         struct packed_git **in_pack;
>
>         pthread_mutex_t lock;
> -       pthread_mutex_t read_lock;

"lock" without any comments in this struct, to me, implies that it protects access to this struct alone. But since you're using it as "read lock" (aka access to object database), I think you should add a comment here clarify the new role of "lock" variable.

-- 
Duy
Junio C Hamano· Jan 24, 2019, 19:48 UTC · re: Duy Nguyen · lore

Re: [PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct

Duy Nguyen <pclouds@gmail.com> writes:
Show 15 quoted lines
> On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
>> diff --git a/pack-objects.h b/pack-objects.h
>> index 0a038e3bc..dc869f26c 100644
>> --- a/pack-objects.h
>> +++ b/pack-objects.h
>> @@ -146,7 +146,6 @@ struct packing_data {
>>         struct packed_git **in_pack;
>>
>>         pthread_mutex_t lock;
>> -       pthread_mutex_t read_lock;
>
> "lock" without any comments in this struct, to me, implies that it
> protects access to this struct alone. But since you're using it as
> "read lock" (aka access to object database), I think you should add a
> comment here clarify the new role of "lock" variable.
Sounds sensible.  How about squashing something like this in?

Some older part of this file still tries to hide the reliance on the global variable "to_pack", but newer code refers to it already, and I think it no longer is buying us much.

 builtin/pack-objects.c | 24 ++++++++++--------------
 pack-objects.c         |  2 +-
 pack-objects.h         | 11 ++++++++---
 3 files changed, 19 insertions(+), 18 deletions(-)
Show changes to 3 files +19 −18

builtin/pack-objects.c, pack-objects.c, pack-objects.h

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 5439b434c6..cfef48217f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1953,10 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 	return 0;
 }
 
-/* Protect access to object database */
-#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;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
@@ -1992,11 +1988,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) {
-		pack_lock();
+		packing_data_lock(&to_pack);
 		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
 			die(_("unable to get size of %s"),
 			    oid_to_hex(&e->idx.oid));
-		pack_unlock();
+		packing_data_unlock(&to_pack);
 		return size;
 	}
 
@@ -2004,7 +2000,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");
 
-	pack_lock();
+	packing_data_lock(&to_pack);
 	w_curs = NULL;
 	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
 	used = unpack_object_header_buffer(buf, avail, &type, &size);
@@ -2013,7 +2009,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 		    oid_to_hex(&e->idx.oid));
 
 	unuse_pack(&w_curs);
-	pack_unlock();
+	packing_data_unlock(&to_pack);
 	return size;
 }
 
@@ -2075,9 +2071,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 
 	/* Load data if not already done */
 	if (!trg->data) {
-		pack_lock();
+		packing_data_lock(&to_pack);
 		trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);
-		pack_unlock();
+		packing_data_unlock(&to_pack);
 		if (!trg->data)
 			die(_("object %s cannot be read"),
 			    oid_to_hex(&trg_entry->idx.oid));
@@ -2088,9 +2084,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 		*mem_usage += sz;
 	}
 	if (!src->data) {
-		pack_lock();
+		packing_data_lock(&to_pack);
 		src->data = read_object_file(&src_entry->idx.oid, &type, &sz);
-		pack_unlock();
+		packing_data_unlock(&to_pack);
 		if (!src->data) {
 			if (src_entry->preferred_base) {
 				static int warned = 0;
@@ -2336,9 +2332,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 
 static void try_to_free_from_threads(size_t size)
 {
-	pack_lock();
+	packing_data_lock(&to_pack);
 	release_pack_memory(size);
-	pack_unlock();
+	packing_data_unlock(&to_pack);
 }
 
 static try_to_free_t old_try_to_free_routine;
diff --git a/pack-objects.c b/pack-objects.c
index 6f32a7ba04..a1dc5eb726 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -148,7 +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);
-	init_recursive_mutex(&pdata->lock);
+	init_recursive_mutex(&pdata->odb_lock);
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
diff --git a/pack-objects.h b/pack-objects.h
index dc869f26c2..a0c21959b2 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -145,7 +145,11 @@ struct packing_data {
 	struct packed_git **in_pack_by_idx;
 	struct packed_git **in_pack;
 
-	pthread_mutex_t lock;
+	/* 
+	 * During packing with multiple threads, protect the in-core
+	 * object database from concurrent accesses.
+	 */
+	pthread_mutex_t odb_lock;
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -165,13 +169,14 @@ struct packing_data {
 
 void prepare_packing_data(struct packing_data *pdata);
 
+/* Protect access to object database */
 static inline void packing_data_lock(struct packing_data *pdata)
 {
-	pthread_mutex_lock(&pdata->lock);
+	pthread_mutex_lock(&pdata->odb_lock);
 }
 static inline void packing_data_unlock(struct packing_data *pdata)
 {
-	pthread_mutex_unlock(&pdata->lock);
+	pthread_mutex_unlock(&pdata->odb_lock);
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
Patrick Hogg· Jan 25, 2019, 00:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct

On Thu, Jan 24, 2019 at 2:49 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
> >> diff --git a/pack-objects.h b/pack-objects.h
> >> index 0a038e3bc..dc869f26c 100644
> >> --- a/pack-objects.h
> >> +++ b/pack-objects.h
> >> @@ -146,7 +146,6 @@ struct packing_data {
> >>         struct packed_git **in_pack;
> >>
> >>         pthread_mutex_t lock;
> >> -       pthread_mutex_t read_lock;
> >
> > "lock" without any comments in this struct, to me, implies that it
> > protects access to this struct alone. But since you're using it as
> > "read lock" (aka access to object database), I think you should add a
> > comment here clarify the new role of "lock" variable.
>
> Sounds sensible.  How about squashing something like this in?
>
> Some older part of this file still tries to hide the reliance on the
> global variable "to_pack", but newer code refers to it already, and
> I think it no longer is buying us much.
>
>  [snip]

Sounds good to me, I'm going to shamelessly use exactly that (with a Thanks-to:) as I have no better suggestions for comments.

Thanks all for the support in my first ever git.git patch!
Duy Nguyen· Jan 24, 2019, 08:35 UTC · re: Patrick Hogg · lore

Re: [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct

On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
Show 11 quoted lines
>
> 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 moving the read mutex to packing_data and initializing
> it in prepare_packing_data which is initialized in cmd_pack_objects.
Obviously correct.
Reviewed-by: Duy Nguyen <pclouds@gmail.com>
Show 83 quoted lines
>
> Signed-off-by: Patrick Hogg <phogg@novamoon.net>
> ---
>  builtin/pack-objects.c |  7 ++-----
>  pack-objects.c         |  1 +
>  pack-objects.h         | 10 ++++++++++
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 411aefd68..506061b4c 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 read_lock()            packing_data_read_lock(&to_pack)
> +#define read_unlock()          packing_data_read_unlock(&to_pack)
>
>  /* Protect delta_cache_size */
>  static pthread_mutex_t cache_mutex;
> @@ -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..3554c43ac 100644
> --- a/pack-objects.c
> +++ b/pack-objects.c
> @@ -150,6 +150,7 @@ void prepare_packing_data(struct packing_data *pdata)
>                                                    1UL << OE_DELTA_SIZE_BITS);
>  #ifndef NO_PTHREADS
>         pthread_mutex_init(&pdata->lock, NULL);
> +       init_recursive_mutex(&pdata->read_lock);
>  #endif
>  }
>
> diff --git a/pack-objects.h b/pack-objects.h
> index dc869f26c..0a038e3bc 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -146,6 +146,7 @@ struct packing_data {
>         struct packed_git **in_pack;
>
>         pthread_mutex_t lock;
> +       pthread_mutex_t read_lock;
>
>         /*
>          * This list contains entries for bases which we know the other side
> @@ -174,6 +175,15 @@ static inline void packing_data_unlock(struct packing_data *pdata)
>         pthread_mutex_unlock(&pdata->lock);
>  }
>
> +static inline void packing_data_read_lock(struct packing_data *pdata)
> +{
> +       pthread_mutex_lock(&pdata->read_lock);
> +}
> +static inline void packing_data_read_unlock(struct packing_data *pdata)
> +{
> +       pthread_mutex_unlock(&pdata->read_lock);
> +}
> +
>  struct object_entry *packlist_alloc(struct packing_data *pdata,
>                                     const unsigned char *sha1,
>                                     uint32_t index_pos);
> --
> 2.20.1.windows.1
>
-- 
Duy

← back to recent threads