File 5451-Avoid-truncating-thread-names-for-better-r... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 5451-Avoid-truncating-thread-names-for-better-runtime-obs.patch of Package erlang (Revision cb0c7ad891d5307a2eb53842d7d69a0f)

Currently displaying revision cb0c7ad891d5307a2eb53842d7d69a0f , Show latest

From bc959f99652dbba79c60f7d062a127b48a276d0e Mon Sep 17 00:00:00 2001
From: lexprfuncall <carl.shapiro@gmail.com>
Date: Wed, 1 Mar 2023 18:45:00 -0800
Subject: [PATCH] Avoid truncating thread names for better runtime
 observability

The Erlang runtime gives many of its threads descriptive names.  When
those threads are part of a logical group, a unique ID is added to the
name for disambiguation.  After construction, many thread names in
Erlang have a string length greater than 16 characters.  To fit within
operating system limits, Erlang then truncates them from right to
left.  To minimize confusion after truncation, the unique ID is always
placed at the left of a thread name so its information is not likely
to be lost.

The convention used by Erlang presents challenges to the use of thread
names as keys when reporting on thread activity at the operating
system level.  The more common convention, used by other runtimes, is
to have the description followed by a unique ID.  When followed,
sorting threads by name places like workers next to each other and the
unique ID can be dropped to create a grouping key. Placing the unique
ID first, as Erlang does, means that a different strategy needs to be
used for sorting the threads of an Erlang process.  Furthermore, the
truncation necessitates a complicated strategy for analyzing the
description to identify a possible common substring to be used as a
grouping key.

This change switches the Erlang runtime to use the more common
convention in order to make reporting on the thread usage in an Erlang
process easier for tooling.  To do so, it shortens the content of the
initial printf(3) format strings to ensure their output is always 16
or fewer characters so the name is never truncated.  It also moves the
unique ID in the format string to the right of the description, so the
names of worker threads appear next to each other after sorting
alphabetically from left to right.

To prevent the accidental creation of long thread names in the future,
the silent truncation has been eliminated from the lowest-layer of
thread functionality.  It now returns an EINVAL when given a long name
which will be caught when the runtime is started.  This would break
the NIF and driver libraries so the silent truncation has moved up to
a higher layer in order to preserve compatibility.  New unit tests have
been added to test setting and getting thread names.
---
 erts/emulator/beam/erl_async.c               |   2 +-
 erts/emulator/beam/erl_drv_thread.c          |   4 +-
 erts/emulator/beam/erl_process.c             |  14 +--
 erts/emulator/beam/erl_threads.h             |   8 ++
 erts/emulator/beam/erl_trace.c               |   2 +-
 erts/emulator/sys/unix/sys.c                 |   5 +-
 erts/include/internal/ethread.h              |   3 +-
 erts/lib_src/pthread/ethread.c               |  22 +---
 erts/test/ethread_SUITE.erl                  |  10 +-
 erts/test/ethread_SUITE_data/ethread_tests.c | 119 ++++++++++++++++++-
 10 files changed, 157 insertions(+), 32 deletions(-)

diff --git a/erts/emulator/beam/erl_async.c b/erts/emulator/beam/erl_async.c
index 3e3dc3a29d..85bc8ee853 100644
--- a/erts/emulator/beam/erl_async.c
+++ b/erts/emulator/beam/erl_async.c
@@ -199,7 +199,7 @@ erts_init_async(void)
 	for (i = 0; i < erts_async_max_threads; i++) {
 	    ErtsAsyncQ *aq = async_q(i);
 
-            erts_snprintf(thr_opts.name, sizeof(thr_name), "async_%d", i+1);
+            erts_snprintf(thr_opts.name, sizeof(thr_name), "erts_async_%d", i+1);
 
 	    erts_thr_create(&aq->thr_id, async_main, (void*) aq, &thr_opts);
 	}
diff --git a/erts/emulator/beam/erl_drv_thread.c b/erts/emulator/beam/erl_drv_thread.c
index 949d89232a..a17b98d1e6 100644
--- a/erts/emulator/beam/erl_drv_thread.c
+++ b/erts/emulator/beam/erl_drv_thread.c
@@ -609,6 +609,7 @@ erl_drv_thread_create(char *name,
     struct ErlDrvTid_ *dtid;
     ethr_thr_opts ethr_opts = ETHR_THR_OPTS_DEFAULT_INITER;
     ethr_thr_opts *use_opts;
+    char name_buff[ETHR_THR_NAME_MAX + 1];
 
     if (!opts && !name)
 	use_opts = NULL;
@@ -616,7 +617,8 @@ erl_drv_thread_create(char *name,
 	if(opts)
 	    ethr_opts.suggested_stack_size = opts->suggested_stack_size;
 
-        ethr_opts.name = name;
+	erts_snprintf(name_buff, sizeof(name_buff), "%s", name);
+	ethr_opts.name = name_buff;
 	use_opts = &ethr_opts;
     }
 
diff --git a/erts/emulator/beam/erl_process.c b/erts/emulator/beam/erl_process.c
index c2ca5a03f3..e8ca466571 100644
--- a/erts/emulator/beam/erl_process.c
+++ b/erts/emulator/beam/erl_process.c
@@ -8776,7 +8776,7 @@ erts_start_schedulers(void)
 {
     ethr_tid tid;
     int res = 0;
-    char name[32];
+    char name[ETHR_THR_NAME_MAX + 1];
     ethr_thr_opts opts = ETHR_THR_OPTS_DEFAULT_INITER;
     int ix;
 
@@ -8786,7 +8786,7 @@ erts_start_schedulers(void)
 
     if (erts_runq_supervision_interval) {
 	opts.suggested_stack_size = 16;
-        erts_snprintf(opts.name, sizeof(name), "runq_supervisor");
+        erts_snprintf(opts.name, sizeof(name), "erts_runq_sup");
 	erts_atomic_init_nob(&runq_supervisor_sleeping, 0);
 	if (0 != ethr_event_init(&runq_supervision_event))
 	    erts_exit(ERTS_ABORT_EXIT, "Failed to create run-queue supervision event\n");
@@ -8807,7 +8807,7 @@ erts_start_schedulers(void)
     for (ix = 0; ix < erts_no_schedulers; ix++) {
 	ErtsSchedulerData *esdp = ERTS_SCHEDULER_IX(ix);
 	ASSERT(ix == esdp->no - 1);
-	erts_snprintf(opts.name, sizeof(name), "%lu_scheduler", ix + 1);
+	erts_snprintf(opts.name, sizeof(name), "erts_sched_%d", ix + 1);
 	res = ethr_thr_create(&esdp->tid, sched_thread_func, (void*)esdp, &opts);
 	if (res != 0) {
            erts_exit(ERTS_ABORT_EXIT, "Failed to create scheduler thread %d, error = %d\n", ix, res);
@@ -8821,7 +8821,7 @@ erts_start_schedulers(void)
     {
 	for (ix = 0; ix < erts_no_dirty_cpu_schedulers; ix++) {
 	    ErtsSchedulerData *esdp = ERTS_DIRTY_CPU_SCHEDULER_IX(ix);
-	    erts_snprintf(opts.name, sizeof(name), "%d_dirty_cpu_scheduler", ix + 1);
+	    erts_snprintf(opts.name, sizeof(name), "erts_dcpus_%d", ix + 1);
             opts.suggested_stack_size = erts_dcpu_sched_thread_suggested_stack_size;
 	    res = ethr_thr_create(&esdp->tid,sched_dirty_cpu_thread_func,(void*)esdp,&opts);
 	    if (res != 0)
@@ -8829,7 +8829,7 @@ erts_start_schedulers(void)
 	}
 	for (ix = 0; ix < erts_no_dirty_io_schedulers; ix++) {
 	    ErtsSchedulerData *esdp = ERTS_DIRTY_IO_SCHEDULER_IX(ix);
-	    erts_snprintf(opts.name, sizeof(name), "%d_dirty_io_scheduler", ix + 1);
+	    erts_snprintf(opts.name, sizeof(name), "erts_dios_%d", ix + 1);
             opts.suggested_stack_size = erts_dio_sched_thread_suggested_stack_size;
 	    res = ethr_thr_create(&esdp->tid,sched_dirty_io_thread_func,(void*)esdp,&opts);
 	    if (res != 0)
@@ -8840,7 +8840,7 @@ erts_start_schedulers(void)
     ix = 0;
     while (ix < erts_no_aux_work_threads) {
 	int id = ix == 0 ? 1 : ix + 1 - (int) erts_no_schedulers;
-	erts_snprintf(opts.name, sizeof(name), "%d_aux", id);
+	erts_snprintf(opts.name, sizeof(name), "erts_aux_%d", id);
 
 	res = ethr_thr_create(&tid, aux_thread, (void *) (Sint) ix, &opts);
 	if (res != 0)
@@ -8867,7 +8867,7 @@ erts_start_schedulers(void)
         bpt->blocked = 0;
         bpt->id = ix;
         
-        erts_snprintf(opts.name, sizeof(name), "%d_poller", ix);
+        erts_snprintf(opts.name, sizeof(name), "erts_poll_%d", ix);
 
         res = ethr_thr_create(&tid, poll_thread, (void*) bpt, &opts);
         if (res != 0)
diff --git a/erts/emulator/beam/erl_threads.h b/erts/emulator/beam/erl_threads.h
index ac3d0d73aa..41f64b93a4 100644
--- a/erts/emulator/beam/erl_threads.h
+++ b/erts/emulator/beam/erl_threads.h
@@ -430,6 +430,7 @@ ERTS_GLB_INLINE void erts_thr_exit(void *res);
 ERTS_GLB_INLINE void erts_thr_install_exit_handler(void (*exit_handler)(void));
 ERTS_GLB_INLINE erts_tid_t erts_thr_self(void);
 ERTS_GLB_INLINE int erts_thr_getname(erts_tid_t tid, char *buf, size_t len);
+ERTS_GLB_INLINE void erts_thr_setname(char *buf);
 ERTS_GLB_INLINE int erts_equal_tids(erts_tid_t x, erts_tid_t y);
 ERTS_GLB_INLINE void erts_mtx_init(erts_mtx_t *mtx,
                                    const char *name,
@@ -1623,6 +1624,13 @@ erts_thr_getname(erts_tid_t tid, char *buf, size_t len)
     return ethr_getname(tid, buf, len);
 }
 
+ERTS_GLB_INLINE void
+erts_thr_setname(char *buf)
+{
+    if (strlen(buf) > ETHR_THR_NAME_MAX)
+	erts_thr_fatal_error(EINVAL, "too long thread name");
+    ethr_setname(buf);
+}
 
 ERTS_GLB_INLINE int
 erts_equal_tids(erts_tid_t x, erts_tid_t y)
diff --git a/erts/emulator/beam/erl_trace.c b/erts/emulator/beam/erl_trace.c
index 82d4cf728c..6d572ab9f3 100644
--- a/erts/emulator/beam/erl_trace.c
+++ b/erts/emulator/beam/erl_trace.c
@@ -2473,7 +2473,7 @@ init_sys_msg_dispatcher(void)
 {
     erts_thr_opts_t thr_opts = ERTS_THR_OPTS_DEFAULT_INITER;
     thr_opts.detached = 1;
-    thr_opts.name = "sys_msg_dispatcher";
+    thr_opts.name = "erts_smsg_disp";
     init_smq_element_alloc();
     sys_message_queue = NULL;
     sys_message_queue_end = NULL;
diff --git a/erts/emulator/sys/unix/sys.c b/erts/emulator/sys/unix/sys.c
index 7ff8425d52..4af776904f 100644
--- a/erts/emulator/sys/unix/sys.c
+++ b/erts/emulator/sys/unix/sys.c
@@ -1057,7 +1057,7 @@ init_smp_sig_notify(void)
 {
     erts_thr_opts_t thr_opts = ERTS_THR_OPTS_DEFAULT_INITER;
     thr_opts.detached = 1;
-    thr_opts.name = "sys_sig_dispatcher";
+    thr_opts.name = "erts_ssig_disp";
 
     if (pipe(sig_notify_fds) < 0) {
 	erts_exit(ERTS_ABORT_EXIT,
@@ -1107,9 +1107,10 @@ erts_sys_main_thread(void)
 #else
     /* Become signal receiver thread... */
 #ifdef ERTS_ENABLE_LOCK_CHECK
-    erts_lc_set_thread_name("signal_receiver");
+    erts_lc_set_thread_name("main");
 #endif
 #endif
+    erts_thr_setname("erts_main");
     smp_sig_notify(0); /* Notify initialized */
 
     /* Wait for a signal to arrive... */
diff --git a/erts/include/internal/ethread.h b/erts/include/internal/ethread.h
index bba35ef02a..b589a54d39 100644
--- a/erts/include/internal/ethread.h
+++ b/erts/include/internal/ethread.h
@@ -497,10 +497,11 @@ typedef struct {
 typedef struct {
     int detached;			/* boolean (default false) */
     int suggested_stack_size;		/* kilo words (default sys dependent) */
-    char *name;                         /* max 14 char long (default no-name) */
+    char *name;                         /* max 15 char long (default no-name) */
 } ethr_thr_opts;
 
 #define ETHR_THR_OPTS_DEFAULT_INITER {0, -1, NULL}
+#define ETHR_THR_NAME_MAX 15
 
 #if !defined(ETHR_TRY_INLINE_FUNCS) || defined(ETHR_AUX_IMPL__)
 #  define ETHR_NEED_SPINLOCK_PROTOTYPES__
diff --git a/erts/lib_src/pthread/ethread.c b/erts/lib_src/pthread/ethread.c
index b17aa3a17f..bfacaa27d0 100644
--- a/erts/lib_src/pthread/ethread.c
+++ b/erts/lib_src/pthread/ethread.c
@@ -81,7 +81,7 @@ typedef struct {
     void *prep_func_res;
     size_t stacksize;
     char *name;
-    char name_buff[32];
+    char name_buff[ETHR_THR_NAME_MAX + 1];
 } ethr_thr_wrap_data__;
 
 static void *thr_wrapper(void *vtwd)
@@ -334,21 +334,9 @@ ethr_thr_create(ethr_tid *tid, void * (*func)(void *), void *arg,
     twd.stacksize = 0;
 
     if (opts && opts->name) {
-        size_t nlen = sizeof(twd.name_buff);
-#ifdef __HAIKU__
-        if (nlen > B_OS_NAME_LENGTH)
-            nlen = B_OS_NAME_LENGTH;
-#else
-        /*
-         * Length of 16 is known to work. At least pthread_setname_np()
-         * is documented to fail on too long name string, but documentation
-         * does not say what the limit is. Do not have the time to dig
-         * further into that now...
-         */
-        if (nlen > 16)
-            nlen = 16;
-#endif
-        snprintf(twd.name_buff, nlen, "%s", opts->name);
+	if (strlen(opts->name) >= sizeof(twd.name_buff))
+	    return EINVAL;
+	strcpy(twd.name_buff, opts->name);
 	twd.name = twd.name_buff;
     } else
         twd.name = NULL;
@@ -506,6 +494,8 @@ ethr_getname(ethr_tid tid, char *buf, size_t len)
 void
 ethr_setname(char *name)
 {
+    if (strlen(name) > ETHR_THR_NAME_MAX)
+        return;
 #if defined(ETHR_HAVE_PTHREAD_SETNAME_NP_2) 
     pthread_setname_np(ethr_self(), name);
 #elif defined(ETHR_HAVE_PTHREAD_SET_NAME_NP_2)
diff --git a/erts/test/ethread_SUITE.erl b/erts/test/ethread_SUITE.erl
index 19f738c572..e5a5e3dba2 100644
--- a/erts/test/ethread_SUITE.erl
+++ b/erts/test/ethread_SUITE.erl
@@ -43,7 +43,8 @@
 	 rwspinlock/1,
 	 rwmutex/1,
 	 atomic/1,
-	 dw_atomic_massage/1]).
+	 dw_atomic_massage/1,
+	 thread_name/1]).
 
 -include_lib("common_test/include/ct.hrl").
 
@@ -65,7 +66,8 @@ all() ->
      rwspinlock,
      rwmutex,
      atomic,
-     dw_atomic_massage].
+     dw_atomic_massage,
+     thread_name].
 
 init_per_testcase(Case, Config) ->
     case inet:gethostname() of
@@ -158,6 +160,10 @@ atomic(Config) ->
 dw_atomic_massage(Config) ->
     run_case(Config, "dw_atomic_massage", "").
 
+%% Tests thread names.
+thread_name(Config) ->
+    run_case(Config, "thread_name", "").
+
 %%
 %%
 %% Auxiliary functions
diff --git a/erts/test/ethread_SUITE_data/ethread_tests.c b/erts/test/ethread_SUITE_data/ethread_tests.c
index 048acd6a95..fcff68a294 100644
--- a/erts/test/ethread_SUITE_data/ethread_tests.c
+++ b/erts/test/ethread_SUITE_data/ethread_tests.c
@@ -41,7 +41,7 @@
 
 #define PRINT_VA_LIST(FRMT)						\
 do {									\
-    if (FRMT && FRMT != '\0') {						\
+    if (FRMT && *(FRMT) != '\0') {					\
 	va_list args;							\
 	va_start(args, FRMT);						\
 	vfprintf(stderr, FRMT, args);					\
@@ -1757,6 +1757,7 @@ at_dw_thr(void *vval)
 		break;
 	}
     }
+    return NULL;
 }
 
 static void
@@ -1783,6 +1784,120 @@ dw_atomic_massage_test(void)
     }
 }
 
+static ethr_mutex thread_name_mutex;
+static ethr_cond thread_name_cond;
+static int thread_name_state;
+
+static void *
+thread_name_thread(void *my_tid)
+{
+    int res;
+
+    ethr_mutex_lock(&thread_name_mutex);
+    thread_name_state = 1;
+    while (thread_name_state == 1) {
+	res = ethr_cond_wait(&thread_name_cond, &thread_name_mutex);
+	ASSERT(res == 0);
+    }
+    ethr_mutex_unlock(&thread_name_mutex);
+    return NULL;
+}
+
+static void
+thread_name(void)
+{
+    static const ethr_thr_opts default_thr_opts = ETHR_THR_OPTS_DEFAULT_INITER;
+    ethr_tid tid;
+    ethr_thr_opts thr_opts;
+    int res;
+    char buf[ETHR_THR_NAME_MAX + 1];
+
+    res = ethr_mutex_init(&thread_name_mutex);
+    ASSERT(res == 0);
+    res = ethr_cond_init(&thread_name_cond);
+    ASSERT(res == 0);
+
+    if (ethr_getname(ethr_self(), buf, sizeof(buf)) == ENOSYS) {
+        skip("thread names are not supported");
+        return;
+    }
+
+    /* create a thread with the minimum name length */
+    thread_name_state = 0;
+
+    memcpy(&thr_opts, &default_thr_opts, sizeof(thr_opts));
+    thr_opts.name = "";
+    res = ethr_thr_create(&tid, thread_name_thread, NULL, &thr_opts);
+    ASSERT(res == 0);
+
+    memset(buf, 0xFF, sizeof(buf));
+    res = ethr_getname(tid, buf, sizeof(buf));
+    ASSERT(res == 0);
+
+    res = strcmp(buf, "");
+    ASSERT(res == 0);
+
+    ethr_mutex_lock(&thread_name_mutex);
+    thread_name_state = 0;
+    ethr_cond_signal(&thread_name_cond);
+    ethr_mutex_unlock(&thread_name_mutex);
+
+    res = ethr_thr_join(tid, NULL);
+    ASSERT(res == 0);
+
+    /* create a thread with a middling name length */
+    thread_name_state = 0;
+
+    memcpy(&thr_opts, &default_thr_opts, sizeof(thr_opts));
+    thr_opts.name = "123456789";
+    res = ethr_thr_create(&tid, thread_name_thread, NULL, &thr_opts);
+    ASSERT(res == 0);
+
+    memset(buf, 0xFF, sizeof(buf));
+    res = ethr_getname(tid, buf, sizeof(buf));
+    ASSERT(res == 0);
+
+    res = strcmp(buf, "123456789");
+    ASSERT(res == 0);
+
+    ethr_mutex_lock(&thread_name_mutex);
+    thread_name_state = 0;
+    ethr_cond_signal(&thread_name_cond);
+    ethr_mutex_unlock(&thread_name_mutex);
+
+    res = ethr_thr_join(tid, NULL);
+    ASSERT(res == 0);
+
+    /* create a thread with the maximum name length */
+    thread_name_state = 0;
+
+    memcpy(&thr_opts, &default_thr_opts, sizeof(thr_opts));
+    thr_opts.name = "123456789012345";
+    res = ethr_thr_create(&tid, thread_name_thread, NULL, &thr_opts);
+    ASSERT(res == 0);
+
+    memset(buf, 0xFF, sizeof(buf));
+    res = ethr_getname(tid, buf, sizeof(buf));
+    ASSERT(res == 0);
+
+    res = strcmp(buf, "123456789012345");
+    ASSERT(res == 0);
+
+    ethr_mutex_lock(&thread_name_mutex);
+    thread_name_state = 0;
+    ethr_cond_signal(&thread_name_cond);
+    ethr_mutex_unlock(&thread_name_mutex);
+
+    res = ethr_thr_join(tid, NULL);
+    ASSERT(res == 0);
+
+    /* create a thread with an over-sized name length */
+    memcpy(&thr_opts, &default_thr_opts, sizeof(thr_opts));
+    thr_opts.name = "1234567890123456";
+    res = ethr_thr_create(&tid, thread_name_thread, NULL, &thr_opts);
+    ASSERT(res == EINVAL);
+}
+
 void *
 at_thread(void *unused)
 {
@@ -1958,6 +2073,8 @@ main(int argc, char *argv[])
 	    atomic_test();
 	else if (strcmp(testcase, "dw_atomic_massage") == 0)
 	    dw_atomic_massage_test();
+        else if (strcmp(testcase, "thread_name") == 0)
+            thread_name();
 	else
 	    skip("Test case \"%s\" not implemented yet", testcase);
 
-- 
2.35.3

Places

File 5451-Avoid-truncating-thread-names-for-better-runtime-obs.patch of Package erlang (Revision cb0c7ad891d5307a2eb53842d7d69a0f)

Places