[3/9] trace2: defer free of TLS CTX until program exit.

Commit Message

Jeff Hostetler Dec. 20, 2021, 3:01 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Defer freeing of the Trace2 thread CTX data until program exit.
Create a global list of thread CTX data to own the storage.

TLS CTX data is allocated when a thread is created and associated
with that thread.  Previously, that storage was deleted when the
thread exited.  Now we simply disassociate the CTX data from the
thread when it exits and let the global CTX list manage the cleanup.

This will be used by a later commit when we add "counters" and
stopwatch-style "timers" to the Trace2 API.  We will add those
fields to the CTX block and allow threads to efficiently (without
locks) accumulate counter and timer data using TLS.  At program
exit, the main thread can run thru the global list and compute
totals before it frees them.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 trace2/tr2_tls.c | 38 ++++++++++++++++++++++++++++----------
 trace2/tr2_tls.h |  3 ++-
 2 files changed, 30 insertions(+), 11 deletions(-)

Comments

Junio C Hamano Dec. 21, 2021, 7:30 a.m. UTC | #1

"Jeff Hostetler via GitGitGadget" <[e-mail]> writes:

> From: Jeff Hostetler <[e-mail]>
>
> Defer freeing of the Trace2 thread CTX data until program exit.
> Create a global list of thread CTX data to own the storage.
>
> TLS CTX data is allocated when a thread is created and associated
> with that thread.  Previously, that storage was deleted when the
> thread exited.  Now we simply disassociate the CTX data from the
> thread when it exits and let the global CTX list manage the cleanup.

By the way, TLS CTX sounds embarrassingly close and confusing to
some function that we may find in say openssl or some crypto stuff
X-<.  Was there a strong reason to avoid calling these functions and
types something like tr2_thread_ctx instead of tr2tls_thread_ctx?

Jeff Hostetler Dec. 22, 2021, 9:59 p.m. UTC | #2

On 12/21/21 2:30 AM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
> 
>> From: Jeff Hostetler <[e-mail]>
>>
>> Defer freeing of the Trace2 thread CTX data until program exit.
>> Create a global list of thread CTX data to own the storage.
>>
>> TLS CTX data is allocated when a thread is created and associated
>> with that thread.  Previously, that storage was deleted when the
>> thread exited.  Now we simply disassociate the CTX data from the
>> thread when it exits and let the global CTX list manage the cleanup.
> 
> By the way, TLS CTX sounds embarrassingly close and confusing to
> some function that we may find in say openssl or some crypto stuff
> X-<.  Was there a strong reason to avoid calling these functions and
> types something like tr2_thread_ctx instead of tr2tls_thread_ctx?
> 

I hadn't really thought about the term "TLS" in the context
of crypto -- I had "thread local storage" on the brain.  I guess
I've spent too much of my youth using Win32 thread APIs. :-)

Let me take a look at removing those terms.

Jeff

Junio C Hamano Dec. 22, 2021, 10:56 p.m. UTC | #3

Jeff Hostetler <[e-mail]> writes:

> I hadn't really thought about the term "TLS" in the context
> of crypto -- I had "thread local storage" on the brain.  I guess
> I've spent too much of my youth using Win32 thread APIs. :-)
>
> Let me take a look at removing those terms.

Nah, it may be just me.  As long as what TLS stands for is clear in
the context, it is fine.

Jeff Hostetler Dec. 22, 2021, 11:04 p.m. UTC | #4

On 12/22/21 5:56 PM, Junio C Hamano wrote:
> Jeff Hostetler <[e-mail]> writes:
> 
>> I hadn't really thought about the term "TLS" in the context
>> of crypto -- I had "thread local storage" on the brain.  I guess
>> I've spent too much of my youth using Win32 thread APIs. :-)
>>
>> Let me take a look at removing those terms.
> 
> Nah, it may be just me.  As long as what TLS stands for is clear in
> the context, it is fine.
> 

ok thanks.  i took a quick look at scrubbing the
code of TLS and even though most of the uses are
in private (or protected) tr2_*.[ch] files, it
will be a large churn-type change and i'm not
sure it's worth the effort.

thanks
jeff

Johannes Sixt Dec. 23, 2021, 7:38 a.m. UTC | #5

Am 22.12.21 um 23:56 schrieb Junio C Hamano:
> Jeff Hostetler <[e-mail]> writes:
> 
>> I hadn't really thought about the term "TLS" in the context
>> of crypto -- I had "thread local storage" on the brain.  I guess
>> I've spent too much of my youth using Win32 thread APIs. :-)
>>
>> Let me take a look at removing those terms.
> 
> Nah, it may be just me.  As long as what TLS stands for is clear in
> the context, it is fine.

No, really, my first reaction was, too: what the hack has crypto to do
with trace2? Are we now sending around trace output by email?

Please use "TLS" next to "CTX" only when it means "Transport Layer
Security".

-- Hannes

Junio C Hamano Dec. 23, 2021, 6:18 p.m. UTC | #6

Johannes Sixt <[e-mail]> writes:

> Am 22.12.21 um 23:56 schrieb Junio C Hamano:
>> Jeff Hostetler <[e-mail]> writes:
>> 
>>> I hadn't really thought about the term "TLS" in the context
>>> of crypto -- I had "thread local storage" on the brain.  I guess
>>> I've spent too much of my youth using Win32 thread APIs. :-)
>>>
>>> Let me take a look at removing those terms.
>> 
>> Nah, it may be just me.  As long as what TLS stands for is clear in
>> the context, it is fine.
>
> No, really, my first reaction was, too: what the hack has crypto to do
> with trace2? Are we now sending around trace output by email?

Ok, then it is not just me ;-)
>
> Please use "TLS" next to "CTX" only when it means "Transport Layer
> Security".

Jeff Hostetler Dec. 27, 2021, 6:51 p.m. UTC | #7

On 12/23/21 1:18 PM, Junio C Hamano wrote:
> Johannes Sixt <[e-mail]> writes:
> 
>> Am 22.12.21 um 23:56 schrieb Junio C Hamano:
>>> Jeff Hostetler <[e-mail]> writes:
>>>
>>>> I hadn't really thought about the term "TLS" in the context
>>>> of crypto -- I had "thread local storage" on the brain.  I guess
>>>> I've spent too much of my youth using Win32 thread APIs. :-)
>>>>
>>>> Let me take a look at removing those terms.
>>>
>>> Nah, it may be just me.  As long as what TLS stands for is clear in
>>> the context, it is fine.
>>
>> No, really, my first reaction was, too: what the hack has crypto to do
>> with trace2? Are we now sending around trace output by email?
> 
> Ok, then it is not just me ;-)
>>
>> Please use "TLS" next to "CTX" only when it means "Transport Layer
>> Security".

I'll make a note to go thru and remove/change these
terms in a future series rather than mix it in with
this one.

Jeff

Patch

diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index cd8b9f2f0a0..b68d297bf51 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -15,7 +15,16 @@  static uint64_t tr2tls_us_start_process;
 static pthread_mutex_t tr2tls_mutex;
 static pthread_key_t tr2tls_key;
 
-static int tr2_next_thread_id; /* modify under lock */
+/*
+ * This list owns all of the thread-specific CTX data.
+ *
+ * While a thread is alive it is associated with a CTX (owned by this
+ * list) and that CTX is installed in the thread's TLS data area.
+ *
+ * Similarly, `tr2tls_thread_main` points to a CTX contained within
+ * this list.
+ */
+static struct tr2tls_thread_ctx *tr2tls_ctx_list; /* modify under lock */
 
 void tr2tls_start_process_clock(void)
 {
@@ -46,7 +55,12 @@  struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 	ctx->array_us_start = (uint64_t *)xcalloc(ctx->alloc, sizeof(uint64_t));
 	ctx->array_us_start[ctx->nr_open_regions++] = us_thread_start;
 
-	ctx->thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
+	pthread_mutex_lock(&tr2tls_mutex);
+	if (tr2tls_ctx_list)
+		ctx->thread_id = tr2tls_ctx_list->thread_id + 1;
+	ctx->next_ctx = tr2tls_ctx_list;
+	tr2tls_ctx_list = ctx;
+	pthread_mutex_unlock(&tr2tls_mutex);
 
 	if (ctx->thread_id)
 		strbuf_addf(&buf_name, "th%02d:", ctx->thread_id);
@@ -91,15 +105,7 @@  int tr2tls_is_main_thread(void)
 
 void tr2tls_unset_self(void)
 {
-	struct tr2tls_thread_ctx *ctx;
-
-	ctx = tr2tls_get_self();
-
 	pthread_setspecific(tr2tls_key, NULL);
-
-	free(ctx->thread_name);
-	free(ctx->array_us_start);
-	free(ctx);
 }
 
 void tr2tls_push_self(uint64_t us_now)
@@ -163,11 +169,23 @@  void tr2tls_init(void)
 
 void tr2tls_release(void)
 {
+	struct tr2tls_thread_ctx *ctx = tr2tls_ctx_list;
+
 	tr2tls_unset_self();
 	tr2tls_thread_main = NULL;
 
 	pthread_mutex_destroy(&tr2tls_mutex);
 	pthread_key_delete(tr2tls_key);
+
+	while (ctx) {
+		struct tr2tls_thread_ctx *next = ctx->next_ctx;
+
+		free(ctx->thread_name);
+		free(ctx->array_us_start);
+		free(ctx);
+
+		ctx = next;
+	}
 }
 
 int tr2tls_locked_increment(int *p)
diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
index d968da6a679..c6b6c69b25a 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -9,6 +9,7 @@ 
 #define TR2_MAX_THREAD_NAME (24)
 
 struct tr2tls_thread_ctx {
+	struct tr2tls_thread_ctx *next_ctx;
 	char *thread_name;
 	uint64_t *array_us_start;
 	size_t alloc;
@@ -45,7 +46,7 @@  struct tr2tls_thread_ctx *tr2tls_get_self(void);
 int tr2tls_is_main_thread(void);
 
 /*
- * Free our TLS data.
+ * Disassociate thread's TLS CTX data from the thread.
  */
 void tr2tls_unset_self(void);