From: Junio C Hamano <gitster@pobox•com>
To: Atneya Nair <atneya@google•com>
Cc: git@vger•kernel.org, jeffhost@microsoft•com, me@ttaylorr•com,
nasamuffin@google•com, Tanay Abhra <tanayabh@gmail•com>,
Glen Choo <glencbz@gmail•com>
Subject: Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
Date: Tue, 05 Mar 2024 09:08:21 -0800 [thread overview]
Message-ID: <xmqqwmqg38u2.fsf@gitster.g> (raw)
In-Reply-To: <20240305012112.1598053-4-atneya@google.com> (Atneya Nair's message of "Mon, 4 Mar 2024 17:21:12 -0800")
Atneya Nair <atneya@google•com> writes:
> To enable parallel update of the read cache for submodules,
> ce_compare_gitlink must be thread safe (for different objects).
>
> Remove string interning in do_config_from (called from
> repo_submodule_init) and add locking around accessing the ref_store_map.
This step does two independent things, even though they may have
dependencies, i.e., for one to be a solution for the problem it is
tackling, the other may have to be there already. E.g., even after
calls to ce_compare_gitlink() get serialized via a mutex, it may for
some reason not work without giving each kvi.filename its own copy
[*], and if that is the case, you may need to have the "stop
interning" step in a single patch with its own justification, and
then "have mutex around ref_store calls" patch has to come after it.
Side note: I do not know if that is the case myself. I didn't
write this commit, you did. The above is just a sample to
illustrate the expected level of depth to explain your thinking
in the log message.
Or if these two things must happen at the same time, please explain
in the proposed log message why they have to happen in the same
commit. The two paragraphs you wrote there don't explain that, so I
am assuming that it is not the case.
The use of strintern() comes originally from 3df8fd62 (add line
number and file name info to `config_set`, 2014-08-07) by Tanay
Abhra <tanayabh@gmail•com>, and survived a handful of changes
809d8680 (config: pass ctx with config files, 2023-06-28)
a798a56c (config.c: plumb the_reader through callbacks, 2023-03-28)
c97f3ed2 (config.c: plumb config_source through static fns, 2023-03-28)
all of which were done by Glen Choo <glencbz@gmail•com>, so they may
know how safe the change on the config side would be (I still do
not understand why you'd want to do this in the first place, though,
especially if you are protecting the callsites with mutex).
I also think Emily's (who you already have on the "CC:" line) group
wants to libify the config machinery and suspect they may still be
making changes to the code, so you may want to coordinate with them
to avoid duplicated work and overlapping changes.
> Signed-off-by: Atneya Nair <atneya@google•com>
> ---
>
> Notes:
> Chasing down thread unsafe code was done using tsan.
Very nice to know.
> config.c | 3 ++-
> config.h | 2 +-
> refs.c | 9 +++++++++
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 3cfeb3d8bd..d7f73d8745 100644
> --- a/config.c
> +++ b/config.c
> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
> enum config_scope scope,
> struct key_value_info *out)
> {
> - out->filename = strintern(cs->name);
> + out->filename = strdup(cs->name);
> out->origin_type = cs->origin_type;
> out->linenr = cs->linenr;
> out->scope = scope;
> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>
> strbuf_release(&top->value);
> strbuf_release(&top->var);
> + free(kvi.filename);
>
> return ret;
> }
> diff --git a/config.h b/config.h
> index 5dba984f77..b78f1b6667 100644
> --- a/config.h
> +++ b/config.h
> @@ -118,7 +118,7 @@ struct config_options {
>
> /* Config source metadata for a given config key-value pair */
> struct key_value_info {
> - const char *filename;
> + char *filename;
> int linenr;
> enum config_origin_type origin_type;
> enum config_scope scope;
> diff --git a/refs.c b/refs.c
> index c633abf284..cce8a31b22 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2126,6 +2126,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> size_t len;
> struct repository *subrepo;
>
> + // TODO is this locking tolerable, and/or can we get any finer
> + static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> +
> if (!submodule)
> return NULL;
>
> @@ -2139,7 +2142,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> /* We need to strip off one or more trailing slashes */
> submodule = to_free = xmemdupz(submodule, len);
>
> + pthread_mutex_lock(&lock);
> refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
> + pthread_mutex_unlock(&lock);
> if (refs)
> goto done;
>
> @@ -2162,10 +2167,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> free(subrepo);
> goto done;
> }
> +
> + pthread_mutex_lock(&lock);
> + // TODO maybe lock this separately
> refs = ref_store_init(subrepo, submodule_sb.buf,
> REF_STORE_READ | REF_STORE_ODB);
> register_ref_store_map(&submodule_ref_stores, "submodule",
> refs, submodule);
> + pthread_mutex_unlock(&lock);
>
> done:
> strbuf_release(&submodule_sb);
next prev parent reply other threads:[~2024-03-05 17:08 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-05 1:21 [RFC PATCH 0/3] Parallel submodule status Atneya Nair
2024-03-05 1:21 ` [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe Atneya Nair
2024-03-05 2:22 ` Junio C Hamano
2024-03-05 4:29 ` Eric Sunshine
2024-03-12 20:38 ` Atneya Nair
2024-03-06 8:13 ` Jean-Noël Avila
2024-03-06 16:57 ` Junio C Hamano
2024-03-12 20:44 ` Atneya Nair
2024-03-05 1:21 ` [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe Atneya Nair
2024-03-05 17:08 ` Junio C Hamano [this message]
2024-03-05 18:53 ` Junio C Hamano
2024-03-06 1:23 ` Jeff King
2024-03-06 1:58 ` Junio C Hamano
2024-03-12 22:13 ` Atneya Nair
2024-03-12 22:15 ` Atneya Nair
2024-03-13 17:51 ` Junio C Hamano
2024-03-05 1:21 ` [RFC PATCH 3/3] Preload submodule state in refresh_index Atneya Nair
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqwmqg38u2.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=atneya@google$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=glencbz@gmail$(echo .)com \
--cc=jeffhost@microsoft$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=nasamuffin@google$(echo .)com \
--cc=tanayabh@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox