Inline the storage for thread name in prof_tdata_t.

The previous approach managed the thread name in a separate buffer, which causes
races because the thread name update (triggered by new samples) can happen at
the same time as prof dumping (which reads the thread names) -- these two
operations are under separate locks to avoid blocking each other.  Implemented
the thread name storage as part of the tdata struct, which resolves the lifetime
issue and also avoids internal alloc / dalloc during prof_sample.

11 files changed, 120 insertions, 103 deletions

diff --git a/include/jemalloc/internal/prof_data.h b/include/jemalloc/internal/prof_data.h
index 4c8e22c7..c4286b51 100644
--- a/include/jemalloc/internal/prof_data.h
+++ b/include/jemalloc/internal/prof_data.h
@@ -18,7 +18,6 @@ bool prof_bt_keycomp(const void *k1, const void *k2);
 
 bool prof_data_init(tsd_t *tsd);
 prof_tctx_t *prof_lookup(tsd_t *tsd, prof_bt_t *bt);
-char *prof_thread_name_alloc(tsd_t *tsd, const char *thread_name);
 int prof_thread_name_set_impl(tsd_t *tsd, const char *thread_name);
 void prof_unbias_map_init();
 void prof_dump_impl(tsd_t *tsd, write_cb_t *prof_dump_write, void *cbopaque,
diff --git a/include/jemalloc/internal/prof_inlines.h b/include/jemalloc/internal/prof_inlines.h
index ab3e01f6..b74b115c 100644
--- a/include/jemalloc/internal/prof_inlines.h
+++ b/include/jemalloc/internal/prof_inlines.h
@@ -38,6 +38,22 @@ prof_gdump_get_unlocked(void) {
 	return prof_gdump_val;
 }
 
+JEMALLOC_ALWAYS_INLINE void
+prof_thread_name_assert(prof_tdata_t *tdata) {
+	if (!config_debug) {
+		return;
+	}
+	prof_active_assert();
+
+	bool terminated = false;
+	for (unsigned i = 0; i < PROF_THREAD_NAME_MAX_LEN; i++) {
+		if (tdata->thread_name[i] == '\0') {
+			terminated = true;
+		}
+	}
+	assert(terminated);
+}
+
 JEMALLOC_ALWAYS_INLINE prof_tdata_t *
 prof_tdata_get(tsd_t *tsd, bool create) {
 	prof_tdata_t *tdata;
@@ -59,6 +75,10 @@ prof_tdata_get(tsd_t *tsd, bool create) {
 		assert(tdata == NULL || tdata->attached);
 	}
 
+	if (tdata != NULL) {
+		prof_thread_name_assert(tdata);
+	}
+
 	return tdata;
 }
 
@@ -255,4 +275,18 @@ prof_free(tsd_t *tsd, const void *ptr, size_t usize,
 	}
 }
 
+JEMALLOC_ALWAYS_INLINE bool
+prof_thread_name_empty(prof_tdata_t *tdata) {
+	prof_active_assert();
+
+	return (tdata->thread_name[0] == '\0');
+}
+
+JEMALLOC_ALWAYS_INLINE void
+prof_thread_name_clear(prof_tdata_t *tdata) {
+	prof_active_assert();
+
+	tdata->thread_name[0] = '\0';
+}
+
 #endif /* JEMALLOC_INTERNAL_PROF_INLINES_H */
diff --git a/include/jemalloc/internal/prof_structs.h b/include/jemalloc/internal/prof_structs.h
index 9331fba4..da3cf8d5 100644
--- a/include/jemalloc/internal/prof_structs.h
+++ b/include/jemalloc/internal/prof_structs.h
@@ -156,9 +156,6 @@ struct prof_tdata_s {
 	 */
 	uint64_t		thr_discrim;
 
-	/* Included in heap profile dumps if non-NULL. */
-	char			*thread_name;
-
 	bool			attached;
 	bool			expired;
 
@@ -179,6 +176,9 @@ struct prof_tdata_s {
 	 */
 	ckh_t			bt2tctx;
 
+	/* Included in heap profile dumps if has content. */
+	char			thread_name[PROF_THREAD_NAME_MAX_LEN];
+
 	/* State used to avoid dumping while operating on prof internals. */
 	bool			enq;
 	bool			enq_idump;
diff --git a/include/jemalloc/internal/prof_types.h b/include/jemalloc/internal/prof_types.h
index 87cbb4ab..104f7e61 100644
--- a/include/jemalloc/internal/prof_types.h
+++ b/include/jemalloc/internal/prof_types.h
@@ -77,4 +77,7 @@ typedef struct prof_recent_s prof_recent_t;
 /* Default number of recent allocations to record. */
 #define PROF_RECENT_ALLOC_MAX_DEFAULT 0
 
+/* Thread name storage size limit. */
+#define PROF_THREAD_NAME_MAX_LEN 16
+
 #endif /* JEMALLOC_INTERNAL_PROF_TYPES_H */
diff --git a/src/ctl.c b/src/ctl.c
index eafbdc61..cfd4ac6e 100644
--- a/src/ctl.c
+++ b/src/ctl.c
@@ -2384,13 +2384,13 @@ thread_prof_name_ctl(tsd_t *tsd, const size_t *mib,
 	READ_XOR_WRITE();
 
 	if (newp != NULL) {
-		if (newlen != sizeof(const char *)) {
+		const char *newval = *(const char **)newp;
+		if (newlen != sizeof(const char *) || newval == NULL) {
 			ret = EINVAL;
 			goto label_return;
 		}
 
-		if ((ret = prof_thread_name_set(tsd, *(const char **)newp)) !=
-		    0) {
+		if ((ret = prof_thread_name_set(tsd, newval)) != 0) {
 			goto label_return;
 		}
 	} else {
diff --git a/src/prof.c b/src/prof.c
index 91425371..832aa528 100644
--- a/src/prof.c
+++ b/src/prof.c
@@ -415,11 +415,14 @@ prof_tdata_t *
 prof_tdata_reinit(tsd_t *tsd, prof_tdata_t *tdata) {
 	uint64_t thr_uid = tdata->thr_uid;
 	uint64_t thr_discrim = tdata->thr_discrim + 1;
-	char *thread_name = (tdata->thread_name != NULL) ?
-	    prof_thread_name_alloc(tsd, tdata->thread_name) : NULL;
 	bool active = tdata->active;
 
+	/* Keep a local copy of the thread name, before detaching. */
+	prof_thread_name_assert(tdata);
+	char thread_name[PROF_THREAD_NAME_MAX_LEN];
+	strncpy(thread_name, tdata->thread_name, PROF_THREAD_NAME_MAX_LEN);
 	prof_tdata_detach(tsd, tdata);
+
 	return prof_tdata_init_impl(tsd, thr_uid, thr_discrim, thread_name,
 	    active);
 }
@@ -464,15 +467,15 @@ prof_active_set(tsdn_t *tsdn, bool active) {
 
 const char *
 prof_thread_name_get(tsd_t *tsd) {
-	assert(tsd_reentrancy_level_get(tsd) == 0);
-
-	prof_tdata_t *tdata;
+	static const char *prof_thread_name_dummy = "";
 
-	tdata = prof_tdata_get(tsd, true);
+	assert(tsd_reentrancy_level_get(tsd) == 0);
+	prof_tdata_t *tdata = prof_tdata_get(tsd, true);
 	if (tdata == NULL) {
-		return "";
+		return prof_thread_name_dummy;
 	}
-	return (tdata->thread_name != NULL ? tdata->thread_name : "");
+
+	return tdata->thread_name;
 }
 
 int
diff --git a/src/prof_data.c b/src/prof_data.c
index 56d3dc88..c33668ee 100644
--- a/src/prof_data.c
+++ b/src/prof_data.c
@@ -441,64 +441,30 @@ prof_bt_count(void) {
 	return bt_count;
 }
 
-char *
-prof_thread_name_alloc(tsd_t *tsd, const char *thread_name) {
-	char *ret;
-	size_t size;
-
-	if (thread_name == NULL) {
-		return NULL;
-	}
-
-	size = strlen(thread_name) + 1;
-	ret = iallocztm(tsd_tsdn(tsd), size, sz_size2index(size), false, NULL,
-	    true, arena_get(TSDN_NULL, 0, true), true);
-	if (ret == NULL) {
-		return NULL;
-	}
-
-	memcpy(ret, thread_name, size);
-	ret[size - 1] = '\0';
-
-	return ret;
+static void
+prof_thread_name_write_tdata(prof_tdata_t *tdata, const char *thread_name) {
+	strncpy(tdata->thread_name, thread_name, PROF_THREAD_NAME_MAX_LEN);
+	tdata->thread_name[PROF_THREAD_NAME_MAX_LEN - 1] = '\0';
 }
 
 int
 prof_thread_name_set_impl(tsd_t *tsd, const char *thread_name) {
 	assert(tsd_reentrancy_level_get(tsd) == 0);
+	assert(thread_name != NULL);
 
-	prof_tdata_t *tdata;
-	unsigned i;
-	char *s;
-
-	tdata = prof_tdata_get(tsd, true);
-	if (tdata == NULL) {
-		return EAGAIN;
-	}
-
-	/* Validate input. */
-	if (thread_name == NULL) {
-		return EFAULT;
-	}
-	for (i = 0; thread_name[i] != '\0'; i++) {
+	for (unsigned i = 0; thread_name[i] != '\0'; i++) {
 		char c = thread_name[i];
 		if (!isgraph(c) && !isblank(c)) {
-			return EFAULT;
+			return EINVAL;
 		}
 	}
 
-	s = prof_thread_name_alloc(tsd, thread_name);
-	if (s == NULL) {
-		return EAGAIN;
+	prof_tdata_t *tdata = prof_tdata_get(tsd, true);
+	if (tdata == NULL) {
+		return ENOMEM;
 	}
 
-	char *old_thread_name = tdata->thread_name;
-	tdata->thread_name = s;
-	if (old_thread_name != NULL) {
-		idalloctm(tsd_tsdn(tsd), old_thread_name, /* tcache */ NULL,
-		    /* alloc_ctx */ NULL, /* is_internal */ true,
-		    /* slow_path */ true);
-	}
+	prof_thread_name_write_tdata(tdata, thread_name);
 
 	return 0;
 }
@@ -949,7 +915,7 @@ prof_tdata_dump_iter(prof_tdata_tree_t *tdatas_ptr, prof_tdata_t *tdata,
 	    tdata->thr_uid);
 	prof_dump_print_cnts(arg->prof_dump_write, arg->cbopaque,
 	    &tdata->cnt_summed);
-	if (tdata->thread_name != NULL) {
+	if (!prof_thread_name_empty(tdata)) {
 		arg->prof_dump_write(arg->cbopaque, " ");
 		arg->prof_dump_write(arg->cbopaque, tdata->thread_name);
 	}
@@ -1179,10 +1145,15 @@ prof_tdata_init_impl(tsd_t *tsd, uint64_t thr_uid, uint64_t thr_discrim,
 	tdata->lock = prof_tdata_mutex_choose(thr_uid);
 	tdata->thr_uid = thr_uid;
 	tdata->thr_discrim = thr_discrim;
-	tdata->thread_name = thread_name;
 	tdata->attached = true;
 	tdata->expired = false;
 	tdata->tctx_uid_next = 0;
+	if (thread_name == NULL) {
+		prof_thread_name_clear(tdata);
+	} else {
+		prof_thread_name_write_tdata(tdata, thread_name);
+	}
+	prof_thread_name_assert(tdata);
 
 	if (ckh_new(tsd, &tdata->bt2tctx, PROF_CKH_MINITEMS, prof_bt_hash,
 	    prof_bt_keycomp)) {
@@ -1230,13 +1201,8 @@ prof_tdata_destroy_locked(tsd_t *tsd, prof_tdata_t *tdata,
 	malloc_mutex_assert_not_owner(tsd_tsdn(tsd), tdata->lock);
 
 	tdata_tree_remove(&tdatas, tdata);
-
 	assert(prof_tdata_should_destroy_unlocked(tdata, even_if_attached));
 
-	if (tdata->thread_name != NULL) {
-		idalloctm(tsd_tsdn(tsd), tdata->thread_name, NULL, NULL, true,
-		    true);
-	}
 	ckh_delete(tsd, &tdata->bt2tctx);
 	idalloctm(tsd_tsdn(tsd), tdata, NULL, NULL, true, true);
 }
diff --git a/src/prof_log.c b/src/prof_log.c
index 0632c3b3..384d5e38 100644
--- a/src/prof_log.c
+++ b/src/prof_log.c
@@ -243,8 +243,7 @@ prof_try_log(tsd_t *tsd, size_t usize, prof_info_t *prof_info) {
 	    iallocztm(tsd_tsdn(tsd), sz, sz_size2index(sz), false, NULL, true,
 	    arena_get(TSDN_NULL, 0, true), true);
 
-	const char *prod_thr_name = (tctx->tdata->thread_name == NULL)?
-				        "" : tctx->tdata->thread_name;
+	const char *prod_thr_name = tctx->tdata->thread_name;
 	const char *cons_thr_name = prof_thread_name_get(tsd);
 
 	prof_bt_t bt;
diff --git a/src/prof_recent.c b/src/prof_recent.c
index 834a9446..4c3c6296 100644
--- a/src/prof_recent.c
+++ b/src/prof_recent.c
@@ -495,7 +495,7 @@ prof_recent_alloc_dump_node(emitter_t *emitter, prof_recent_t *node) {
 	    &node->alloc_tctx->thr_uid);
 	prof_tdata_t *alloc_tdata = node->alloc_tctx->tdata;
 	assert(alloc_tdata != NULL);
-	if (alloc_tdata->thread_name != NULL) {
+	if (!prof_thread_name_empty(alloc_tdata)) {
 		emitter_json_kv(emitter, "alloc_thread_name",
 		    emitter_type_string, &alloc_tdata->thread_name);
 	}
@@ -511,7 +511,7 @@ prof_recent_alloc_dump_node(emitter_t *emitter, prof_recent_t *node) {
 		    emitter_type_uint64, &node->dalloc_tctx->thr_uid);
 		prof_tdata_t *dalloc_tdata = node->dalloc_tctx->tdata;
 		assert(dalloc_tdata != NULL);
-		if (dalloc_tdata->thread_name != NULL) {
+		if (!prof_thread_name_empty(dalloc_tdata)) {
 			emitter_json_kv(emitter, "dalloc_thread_name",
 			    emitter_type_string, &dalloc_tdata->thread_name);
 		}
diff --git a/src/prof_sys.c b/src/prof_sys.c
index d2487fd6..3f7196f8 100644
--- a/src/prof_sys.c
+++ b/src/prof_sys.c
@@ -462,12 +462,17 @@ prof_sys_thread_name_read_t *JET_MUTABLE prof_sys_thread_name_read =
 
 void
 prof_sys_thread_name_fetch(tsd_t *tsd) {
-#define THREAD_NAME_MAX_LEN 16
-	char buf[THREAD_NAME_MAX_LEN];
-	if (!prof_sys_thread_name_read(buf, THREAD_NAME_MAX_LEN)) {
-		prof_thread_name_set_impl(tsd, buf);
+	prof_tdata_t *tdata = prof_tdata_get(tsd, true);
+	if (tdata == NULL) {
+		return;
 	}
-#undef THREAD_NAME_MAX_LEN
+
+	if (prof_sys_thread_name_read(tdata->thread_name,
+	    PROF_THREAD_NAME_MAX_LEN) != 0) {
+		prof_thread_name_clear(tdata);
+	}
+
+	tdata->thread_name[PROF_THREAD_NAME_MAX_LEN - 1] = '\0';
 }
 
 int
diff --git a/test/unit/prof_thread_name.c b/test/unit/prof_thread_name.c
index 3c4614fc..0fc29f75 100644
--- a/test/unit/prof_thread_name.c
+++ b/test/unit/prof_thread_name.c
@@ -14,8 +14,6 @@ mallctl_thread_name_get_impl(const char *thread_name_expected, const char *func,
 	expect_str_eq(thread_name_old, thread_name_expected,
 	    "%s():%d: Unexpected thread.prof.name value", func, line);
 }
-#define mallctl_thread_name_get(a)					\
-	mallctl_thread_name_get_impl(a, __func__, __LINE__)
 
 static void
 mallctl_thread_name_set_impl(const char *thread_name, const char *func,
@@ -26,51 +24,59 @@ mallctl_thread_name_set_impl(const char *thread_name, const char *func,
 	    func, line);
 	mallctl_thread_name_get_impl(thread_name, func, line);
 }
+
+#define mallctl_thread_name_get(a)					\
+	mallctl_thread_name_get_impl(a, __func__, __LINE__)
+
 #define mallctl_thread_name_set(a)					\
 	mallctl_thread_name_set_impl(a, __func__, __LINE__)
 
 TEST_BEGIN(test_prof_thread_name_validation) {
-	const char *thread_name;
-
 	test_skip_if(!config_prof);
 	test_skip_if(opt_prof_sys_thread_name);
 
 	mallctl_thread_name_get("");
-	mallctl_thread_name_set("hi there");
+
+	const char *test_name1 = "test case1";
+	mallctl_thread_name_set(test_name1);
+
+	/* Test name longer than the max len. */
+	char long_name[] =
+	    "test case longer than expected; test case longer than expected";
+	expect_zu_gt(strlen(long_name), PROF_THREAD_NAME_MAX_LEN,
+	   "Long test name not long enough");
+	const char *test_name_long = long_name;
+	expect_d_eq(mallctl("thread.prof.name", NULL, NULL,
+	    (void *)&test_name_long, sizeof(test_name_long)), 0,
+	    "Unexpected mallctl failure from thread.prof.name");
+	/* Long name cut to match. */
+	long_name[PROF_THREAD_NAME_MAX_LEN - 1] = '\0';
+	mallctl_thread_name_get(test_name_long);
 
 	/* NULL input shouldn't be allowed. */
-	thread_name = NULL;
+	const char *test_name2 = NULL;
 	expect_d_eq(mallctl("thread.prof.name", NULL, NULL,
-	    (void *)&thread_name, sizeof(thread_name)), EFAULT,
-	    "Unexpected mallctl result writing \"%s\" to thread.prof.name",
-	    thread_name);
+	    (void *)&test_name2, sizeof(test_name2)), EINVAL,
+	    "Unexpected mallctl result writing to thread.prof.name");
 
 	/* '\n' shouldn't be allowed. */
-	thread_name = "hi\nthere";
+	const char *test_name3 = "test\ncase";
 	expect_d_eq(mallctl("thread.prof.name", NULL, NULL,
-	    (void *)&thread_name, sizeof(thread_name)), EFAULT,
+	    (void *)&test_name3, sizeof(test_name3)), EINVAL,
 	    "Unexpected mallctl result writing \"%s\" to thread.prof.name",
-	    thread_name);
+	    test_name3);
 
 	/* Simultaneous read/write shouldn't be allowed. */
-	{
-		const char *thread_name_old;
-		size_t sz;
-
-		sz = sizeof(thread_name_old);
-		expect_d_eq(mallctl("thread.prof.name",
-		    (void *)&thread_name_old, &sz, (void *)&thread_name,
-		    sizeof(thread_name)), EPERM,
-		    "Unexpected mallctl result writing \"%s\" to "
-		    "thread.prof.name", thread_name);
-	}
+	const char *thread_name_old;
+	size_t sz = sizeof(thread_name_old);
+	expect_d_eq(mallctl("thread.prof.name", (void *)&thread_name_old, &sz,
+	    (void *)&test_name1, sizeof(test_name1)), EPERM,
+	    "Unexpected mallctl result from thread.prof.name");
 
 	mallctl_thread_name_set("");
 }
 TEST_END
 
-#define NTHREADS	4
-#define NRESET		25
 static void *
 thd_start(void *varg) {
 	unsigned thd_ind = *(unsigned *)varg;
@@ -82,6 +88,7 @@ thd_start(void *varg) {
 	mallctl_thread_name_get("");
 	mallctl_thread_name_set(thread_name);
 
+#define NRESET 25
 	for (i = 0; i < NRESET; i++) {
 		expect_d_eq(mallctl("prof.reset", NULL, NULL, NULL, 0), 0,
 		    "Unexpected error while resetting heap profile data");
@@ -92,12 +99,14 @@ thd_start(void *varg) {
 	mallctl_thread_name_set("");
 
 	return NULL;
+#undef NRESET
 }
 
 TEST_BEGIN(test_prof_thread_name_threaded) {
 	test_skip_if(!config_prof);
 	test_skip_if(opt_prof_sys_thread_name);
 
+#define NTHREADS 4
 	thd_t thds[NTHREADS];
 	unsigned thd_args[NTHREADS];
 	unsigned i;
@@ -109,10 +118,9 @@ TEST_BEGIN(test_prof_thread_name_threaded) {
 	for (i = 0; i < NTHREADS; i++) {
 		thd_join(thds[i], NULL);
 	}
+#undef NTHREADS
 }
 TEST_END
-#undef NTHREADS
-#undef NRESET
 
 int
 main(void) {