diff mbox series

[v2,1/4] eal: add thread set name API operating on rte thread

Message ID 1671036441-10234-2-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series add rte_thread_set_name API for rte_thread_t |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Tyler Retzlaff Dec. 14, 2022, 4:47 p.m. UTC 
  Add a rte_thread_set_name that sets the name of an rte_thread_t thread.
This is a replacement for the rte_thread_setname(pthread_t, ...) which
exposes platform-specific details.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/common/eal_common_thread.c |  9 +++-----
 lib/eal/common/eal_common_trace.c  |  2 --
 lib/eal/freebsd/eal.c              |  4 +++-
 lib/eal/freebsd/eal_thread.c       | 11 +++++++++
 lib/eal/include/rte_thread.h       | 17 ++++++++++++++
 lib/eal/linux/eal.c                |  8 +++----
 lib/eal/linux/eal_thread.c         | 22 ++++++++++++++++++
 lib/eal/version.map                |  3 +++
 lib/eal/windows/rte_thread.c       | 46 ++++++++++++++++++++++++++++++++++++++
 9 files changed, 108 insertions(+), 14 deletions(-)

Comments

David Marchand Jan. 11, 2023, 4:09 p.m. UTC | #1 
On Wed, Dec 14, 2022 at 5:47 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index 5caaac8..89522dc 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -356,8 +356,6 @@ rte_trace_mode rte_trace_mode_get(void)
>         /* Store the thread name */
>         char *name = header->stream_header.thread_name;
>         memset(name, 0, __RTE_TRACE_EMIT_STRING_LEN_MAX);
> -       rte_thread_getname(pthread_self(), name,
> -               __RTE_TRACE_EMIT_STRING_LEN_MAX);
>
>         trace->lcore_meta[count].mem = header;
>         trace->nb_trace_mem_list++;

Note, this belongs to patch 2.

I understand we can drop the thread name retrieval helper from public
API, but at least for the trace framework it added some info in the
traces.
Jerin, Sunil, what do you think? Should we keep this helper internally?
Tyler Retzlaff Jan. 12, 2023, 9:32 p.m. UTC | #2 
On Wed, Jan 11, 2023 at 05:09:25PM +0100, David Marchand wrote:
> On Wed, Dec 14, 2022 at 5:47 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> > diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> > index 5caaac8..89522dc 100644
> > --- a/lib/eal/common/eal_common_trace.c
> > +++ b/lib/eal/common/eal_common_trace.c
> > @@ -356,8 +356,6 @@ rte_trace_mode rte_trace_mode_get(void)
> >         /* Store the thread name */
> >         char *name = header->stream_header.thread_name;
> >         memset(name, 0, __RTE_TRACE_EMIT_STRING_LEN_MAX);
> > -       rte_thread_getname(pthread_self(), name,
> > -               __RTE_TRACE_EMIT_STRING_LEN_MAX);
> >
> >         trace->lcore_meta[count].mem = header;
> >         trace->nb_trace_mem_list++;
> 
> Note, this belongs to patch 2.

agreed, not sure how i fumbled that. since there has been no reply about
retaining rte_thread_getname as an internal api i'll just move this
change to patch 2.

> I understand we can drop the thread name retrieval helper from public
> API, but at least for the trace framework it added some info in the
> traces.
> Jerin, Sunil, what do you think? Should we keep this helper internally?
> 
> 
> -- 
> David Marchand
Jerin Jacob Jan. 13, 2023, 4:36 a.m. UTC | #3 
On Wed, Jan 11, 2023 at 9:39 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Wed, Dec 14, 2022 at 5:47 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> > diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> > index 5caaac8..89522dc 100644
> > --- a/lib/eal/common/eal_common_trace.c
> > +++ b/lib/eal/common/eal_common_trace.c
> > @@ -356,8 +356,6 @@ rte_trace_mode rte_trace_mode_get(void)
> >         /* Store the thread name */
> >         char *name = header->stream_header.thread_name;
> >         memset(name, 0, __RTE_TRACE_EMIT_STRING_LEN_MAX);
> > -       rte_thread_getname(pthread_self(), name,
> > -               __RTE_TRACE_EMIT_STRING_LEN_MAX);
> >
> >         trace->lcore_meta[count].mem = header;
> >         trace->nb_trace_mem_list++;
>
> Note, this belongs to patch 2.
>
> I understand we can drop the thread name retrieval helper from public
> API, but at least for the trace framework it added some info in the
> traces.
> Jerin, Sunil, what do you think? Should we keep this helper internally?


Good catch @David Marchand. Yes, we should be it as internal helper
API use it here.
For trace, It will be difficult to make sense without thread name.

>
>
> --
> David Marchand
>
Tyler Retzlaff Jan. 13, 2023, 5:09 p.m. UTC | #4 
On Fri, Jan 13, 2023 at 10:06:44AM +0530, Jerin Jacob wrote:
> On Wed, Jan 11, 2023 at 9:39 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Wed, Dec 14, 2022 at 5:47 PM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > > diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> > > index 5caaac8..89522dc 100644
> > > --- a/lib/eal/common/eal_common_trace.c
> > > +++ b/lib/eal/common/eal_common_trace.c
> > > @@ -356,8 +356,6 @@ rte_trace_mode rte_trace_mode_get(void)
> > >         /* Store the thread name */
> > >         char *name = header->stream_header.thread_name;
> > >         memset(name, 0, __RTE_TRACE_EMIT_STRING_LEN_MAX);
> > > -       rte_thread_getname(pthread_self(), name,
> > > -               __RTE_TRACE_EMIT_STRING_LEN_MAX);
> > >
> > >         trace->lcore_meta[count].mem = header;
> > >         trace->nb_trace_mem_list++;
> >
> > Note, this belongs to patch 2.
> >
> > I understand we can drop the thread name retrieval helper from public
> > API, but at least for the trace framework it added some info in the
> > traces.
> > Jerin, Sunil, what do you think? Should we keep this helper internally?
> 
> 
> Good catch @David Marchand. Yes, we should be it as internal helper
> API use it here.
> For trace, It will be difficult to make sense without thread name.

is the name what is really valuable here because it will only appear in
traces for linux with a new enough glibc, i.e. freebsd and windows it
will always be empty.

we could just provide the thread id and that is available on all
platforms, i'd like to see consistency for all platforms if possible
here but i'm not pushing too hard since we don't declare our tracing
output to be a compatibility surface.

can we just provide the thread id?

> 
> >
> >
> > --
> > David Marchand
> >
Tyler Retzlaff Jan. 13, 2023, 6:56 p.m. UTC | #5 
On Fri, Jan 13, 2023 at 09:09:21AM -0800, Tyler Retzlaff wrote:
> On Fri, Jan 13, 2023 at 10:06:44AM +0530, Jerin Jacob wrote:
> > On Wed, Jan 11, 2023 at 9:39 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Wed, Dec 14, 2022 at 5:47 PM Tyler Retzlaff
> > > <roretzla@linux.microsoft.com> wrote:
> > > > diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> > > > index 5caaac8..89522dc 100644
> > > > --- a/lib/eal/common/eal_common_trace.c
> > > > +++ b/lib/eal/common/eal_common_trace.c
> > > > @@ -356,8 +356,6 @@ rte_trace_mode rte_trace_mode_get(void)
> > > >         /* Store the thread name */
> > > >         char *name = header->stream_header.thread_name;
> > > >         memset(name, 0, __RTE_TRACE_EMIT_STRING_LEN_MAX);
> > > > -       rte_thread_getname(pthread_self(), name,
> > > > -               __RTE_TRACE_EMIT_STRING_LEN_MAX);
> > > >
> > > >         trace->lcore_meta[count].mem = header;
> > > >         trace->nb_trace_mem_list++;
> > >
> > > Note, this belongs to patch 2.
> > >
> > > I understand we can drop the thread name retrieval helper from public
> > > API, but at least for the trace framework it added some info in the
> > > traces.
> > > Jerin, Sunil, what do you think? Should we keep this helper internally?
> > 
> > 
> > Good catch @David Marchand. Yes, we should be it as internal helper
> > API use it here.
> > For trace, It will be difficult to make sense without thread name.
> 
> is the name what is really valuable here because it will only appear in
> traces for linux with a new enough glibc, i.e. freebsd and windows it
> will always be empty.
> 
> we could just provide the thread id and that is available on all
> platforms, i'd like to see consistency for all platforms if possible
> here but i'm not pushing too hard since we don't declare our tracing
> output to be a compatibility surface.
> 
> can we just provide the thread id?

as a compromise i have submitted a new version for the series that
curtails the scope of the original rte_thread_getname and allowing it at
this single callsite.

going forward however i would suggest we do not allow further internal
use.

thanks!
diff mbox series

Patch

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index c5d8b43..a44023c 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -291,12 +291,9 @@  static void *ctrl_thread_init(void *arg)
 		return -ret;
 	}
 
-	if (name != NULL) {
-		ret = rte_thread_setname(*thread, name);
-		if (ret < 0)
-			RTE_LOG(DEBUG, EAL,
-				"Cannot set name for ctrl thread\n");
-	}
+	if (name != NULL)
+		rte_thread_set_name((rte_thread_t){(uintptr_t)*thread},
+			name);
 
 	/* Wait for the control thread to initialize successfully */
 	while ((ctrl_thread_status =
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 5caaac8..89522dc 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -356,8 +356,6 @@  rte_trace_mode rte_trace_mode_get(void)
 	/* Store the thread name */
 	char *name = header->stream_header.thread_name;
 	memset(name, 0, __RTE_TRACE_EMIT_STRING_LEN_MAX);
-	rte_thread_getname(pthread_self(), name,
-		__RTE_TRACE_EMIT_STRING_LEN_MAX);
 
 	trace->lcore_meta[count].mem = header;
 	trace->nb_trace_mem_list++;
diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index 607684c..2a6415e 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -817,7 +817,9 @@  static void rte_eal_init_alert(const char *msg)
 		/* Set thread_name for aid in debugging. */
 		snprintf(thread_name, sizeof(thread_name),
 				"rte-worker-%d", i);
-		rte_thread_setname(lcore_config[i].thread_id, thread_name);
+		rte_thread_set_name(
+			(rte_thread_t){(uintptr_t)lcore_config[i].thread_id},
+			thread_name);
 
 		ret = pthread_setaffinity_np(lcore_config[i].thread_id,
 			sizeof(rte_cpuset_t), &lcore_config[i].cpuset);
diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index ab81b52..b69f5d3 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -32,6 +32,17 @@  int rte_sys_gettid(void)
 	return (int)lwpid;
 }
 
+void rte_thread_set_name(rte_thread_t thread_id, const char *thread_name)
+{
+	char truncated[RTE_MAX_THREAD_NAME_LEN];
+	const size_t truncatedsz = sizeof(truncated);
+
+	if (strlcpy(truncated, thread_name, truncatedsz) >= truncatedsz)
+		RTE_LOG(DEBUG, EAL, "Truncated thread name\n");
+
+	pthread_set_name_np((pthread_t)thread_id.opaque_id, truncated);
+}
+
 int rte_thread_setname(pthread_t id, const char *name)
 {
 	/* this BSD function returns no error */
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index b9edf70..8a63a52 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -146,6 +146,23 @@  int rte_thread_create(rte_thread_t *thread_id,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
+ * Set the name of the thread.
+ *
+ * @param thread_id
+ *    The id of the thread to set name.
+ *
+ * @param thread_name
+ *    The name to set. Truncated to RTE_MAX_THREAD_NAME_LEN,
+ *    including terminating NUL if necessary.
+ */
+__rte_experimental
+void
+rte_thread_set_name(rte_thread_t thread_id, const char *thread_name);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
  * Check if 2 thread ids are equal.
  *
  * @param t1
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 8c118d0..f7a4a10 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1255,11 +1255,9 @@  static void rte_eal_init_alert(const char *msg)
 		/* Set thread_name for aid in debugging. */
 		snprintf(thread_name, sizeof(thread_name),
 			"rte-worker-%d", i);
-		ret = rte_thread_setname(lcore_config[i].thread_id,
-						thread_name);
-		if (ret != 0)
-			RTE_LOG(DEBUG, EAL,
-				"Cannot set name for lcore thread\n");
+		rte_thread_set_name(
+			(rte_thread_t){(uintptr_t)lcore_config[i].thread_id},
+			thread_name);
 
 		ret = pthread_setaffinity_np(lcore_config[i].thread_id,
 			sizeof(rte_cpuset_t), &lcore_config[i].cpuset);
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index 625bde6..d5fddab 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -10,6 +10,7 @@ 
 
 #include <rte_eal.h>
 #include <rte_lcore.h>
+#include <rte_log.h>
 #include <rte_string_fns.h>
 
 /* require calling thread tid by gettid() */
@@ -18,6 +19,27 @@  int rte_sys_gettid(void)
 	return (int)syscall(SYS_gettid);
 }
 
+void rte_thread_set_name(rte_thread_t thread_id, const char *thread_name)
+{
+	int ret = ENOSYS;
+#if defined(__GLIBC__) && defined(__GLIBC_PREREQ)
+#if __GLIBC_PREREQ(2, 12)
+	char truncated[RTE_MAX_THREAD_NAME_LEN];
+	const size_t truncatedsz = sizeof(truncated);
+
+	if (strlcpy(truncated, thread_name, truncatedsz) >= truncatedsz)
+		RTE_LOG(DEBUG, EAL, "Truncated thread name\n");
+
+	ret = pthread_setname_np((pthread_t)thread_id.opaque_id, truncated);
+#endif
+#endif
+	RTE_SET_USED(thread_id);
+	RTE_SET_USED(thread_name);
+
+	if (ret != 0)
+		RTE_LOG(DEBUG, EAL, "Failed to set thread name\n");
+}
+
 int rte_thread_setname(pthread_t id, const char *name)
 {
 	int ret = ENOSYS;
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7..c98ba71 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,9 @@  EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+
+	# added in 23.03
+	rte_thread_set_name;
 };
 
 INTERNAL {
diff --git a/lib/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c
index 1c1e9d0..c26659d 100644
--- a/lib/eal/windows/rte_thread.c
+++ b/lib/eal/windows/rte_thread.c
@@ -4,7 +4,9 @@ 
  */
 
 #include <errno.h>
+#include <wchar.h>
 
+#include <rte_eal.h>
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_thread.h>
@@ -305,6 +307,50 @@  struct thread_routine_ctx {
 	return thread_id;
 }
 
+void
+rte_thread_set_name(rte_thread_t thread_id, const char *thread_name)
+{
+	int ret = 0;
+	wchar_t wname[RTE_MAX_THREAD_NAME_LEN];
+	mbstate_t state = {0};
+	size_t rv;
+	HANDLE thread_handle;
+
+	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE,
+		thread_id.opaque_id);
+	if (thread_handle == NULL) {
+		ret = thread_log_last_error("OpenThread()");
+		goto cleanup;
+	}
+
+	memset(wname, 0, sizeof(wname));
+	rv = mbsrtowcs(wname, &thread_name, RTE_DIM(wname) - 1, &state);
+	if (rv == (size_t)-1) {
+		ret = EILSEQ;
+		goto cleanup;
+	}
+
+	if (wcslen(wname) < strlen(thread_name))
+		RTE_LOG(DEBUG, EAL, "Truncated thread name\n");
+
+#ifndef RTE_TOOLCHAIN_GCC
+	if (FAILED(SetThreadDescription(thread_handle, wname))) {
+		ret = EINVAL;
+		goto cleanup;
+	}
+#else
+	ret = ENOTSUP;
+	goto cleanup;
+#endif
+
+cleanup:
+	if (thread_handle != NULL)
+		CloseHandle(thread_handle);
+
+	if (ret != 0)
+		RTE_LOG(DEBUG, EAL, "Failed to set thread name\n");
+}
+
 int
 rte_thread_get_priority(rte_thread_t thread_id,
 	enum rte_thread_priority *priority)