Re: [PATCH v2 2/9] trace2: convert tr2tls_thread_ctx.thread_name from strbuf to flex array

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>
> Move the thread name to a flex array at the bottom of the Trace2
> thread local storage data and get rid of the strbuf.
>
> Let the flex array have the full computed value of the thread name
> without truncation.
>
> Change the PERF target to truncate the thread name so that the columns
> still line up.

This commit message really doesn't help in explaining what we're trying
to do here and why it's needed. I'm not saying it's not, but why not a
strbuf, why a flex array? The diff below also shows changes unrelated to
this.

I tried this local fixup on top of this series which works, so I wonder
if we're just trying to get rid of the strbuf to signal that this
shouldn't change why not just strbuf_detach() and keep a "const char
*thread_name"?

diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 28ea55863d1..35d49b27b2e 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -48,7 +48,7 @@ void tr2tls_start_process_clock(void)
 struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 					     uint64_t us_thread_start)
 {
-	struct tr2tls_thread_ctx *ctx;
+	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(struct tr2tls_thread_ctx));
 	struct strbuf buf_name = STRBUF_INIT;
 	int thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
 
@@ -56,8 +56,7 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 		strbuf_addf(&buf_name, "th%02d:", thread_id);
 	strbuf_addstr(&buf_name, thread_name);
 
-	FLEX_ALLOC_MEM(ctx, thread_name, buf_name.buf, buf_name.len);
-	strbuf_release(&buf_name);
+	ctx->thread_name = strbuf_detach(&buf_name, NULL);
 
 	ctx->thread_id = thread_id;
 
@@ -188,6 +187,7 @@ void tr2tls_release(void)
 	while (ctx) {
 		struct tr2tls_thread_ctx *next = ctx->next_ctx;
 
+		free((char *)ctx->thread_name);
 		free(ctx->array_us_start);
 		free(ctx);
 
diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
index 503829bbd44..bc6c6f12e38 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -6,6 +6,7 @@
 #include "trace2/tr2_tmr.h"
 
 struct tr2tls_thread_ctx {
+	const char *thread_name;
 	struct tr2tls_thread_ctx *next_ctx;
 	uint64_t *array_us_start;
 	size_t alloc;
@@ -14,8 +15,6 @@ struct tr2tls_thread_ctx {
 
 	struct tr2_timer_block timers;
 	struct tr2_counter_block counters;
-
-	char thread_name[FLEX_ARRAY];
 };
 
 /*

> [...]
> index 7da94aba522..ed99a234b95 100644
> --- a/trace2/tr2_tls.c
> +++ b/trace2/tr2_tls.c
> @@ -34,7 +34,18 @@ void tr2tls_start_process_clock(void)
>  struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
>  					     uint64_t us_thread_start)
>  {
> -	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));
> +	struct tr2tls_thread_ctx *ctx;
> +	struct strbuf buf_name = STRBUF_INIT;
> +	int thread_id = tr2tls_locked_increment(&tr2_next_thread_id);

Here's the looks-to-be-unrelated to this strbuf conversion code I
mentioned above.

> +
> +	if (thread_id)
> +		strbuf_addf(&buf_name, "th%02d:", thread_id);
> +	strbuf_addstr(&buf_name, thread_name);
> +
> +	FLEX_ALLOC_MEM(ctx, thread_name, buf_name.buf, buf_name.len);
> +	strbuf_release(&buf_name);
> +
> +	ctx->thread_id = thread_id;
>  
>  	/*
> [...]