[dpdk-dev] [PATCH] Use pthread_setname APIs

[dpdk-dev] [PATCH] Use pthread_setname APIs

[Ravi Kerur]

Add code to set names to threads via pthread APIs.
In Linux corresponding _getname_ is available, however, FreeBSD
doesn't have corresponding _get_name API available yet. Hence _getname_
is not yet used in the code.

Ravi Kerur (1):
  Use pthread_setname apis

 config/common_bsdapp                               |  5 +++++
 config/common_linuxapp                             |  5 +++++
 examples/vhost/Makefile                            |  1 +
 examples/vhost/main.c                              | 18 ++++++++++++++++--
 examples/vhost_xen/Makefile                        |  1 +
 examples/vhost_xen/main.c                          | 20 ++++++++++++++++++--
 lib/librte_eal/bsdapp/eal/eal.c                    |  7 +++++++
 lib/librte_eal/linuxapp/eal/Makefile               |  2 ++
 lib/librte_eal/linuxapp/eal/eal.c                  | 11 +++++++++++
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 20 +++++++++++++++++---
 lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c | 16 ++++++++++++++--
 lib/librte_eal/linuxapp/eal/eal_timer.c            | 11 ++++++++++-
 12 files changed, 107 insertions(+), 10 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH] Use pthread_setname APIs

[Ravi Kerur]

use pthread_setname_np and pthread_set_name_np for Linux and
FreeBSD respectively.
Restrict pthread name len to 16 via config for both Linux and FreeBSD.

Testing:
Linux:
Compiled with both clang and gcc (x86_64-native-linuxapp-gcc and
x86_64-native-linuxapp-clang).
Compiled examples/vhost.
make test.
testpmd with I217 NIC.
Check /proc/<tid>/comm file for names.

FreeBSD:
Compiled with gcc (x86_64-native-bsdapp-gcc)
helloworld/testpmd with I218 NIC.

Signed-off-by: Ravi Kerur <rkerur@gmail.com>
---
 config/common_bsdapp                               |  5 +++++
 config/common_linuxapp                             |  5 +++++
 examples/vhost/Makefile                            |  1 +
 examples/vhost/main.c                              | 18 ++++++++++++++++--
 examples/vhost_xen/Makefile                        |  1 +
 examples/vhost_xen/main.c                          | 20 ++++++++++++++++++--
 lib/librte_eal/bsdapp/eal/eal.c                    |  7 +++++++
 lib/librte_eal/linuxapp/eal/Makefile               |  2 ++
 lib/librte_eal/linuxapp/eal/eal.c                  | 11 +++++++++++
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 20 +++++++++++++++++---
 lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c | 16 ++++++++++++++--
 lib/librte_eal/linuxapp/eal/eal_timer.c            | 11 ++++++++++-
 12 files changed, 107 insertions(+), 10 deletions(-)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index c2374c0..9cec72b 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -244,6 +244,11 @@ CONFIG_RTE_PMD_RING_MAX_RX_RINGS=16
 CONFIG_RTE_PMD_RING_MAX_TX_RINGS=16
 
 #
+# Max pthread name len
+#
+CONFIG_RTE_MAX_THREAD_NAME_LEN=16
+
+#
 # Compile software PMD backed by PCAP files
 #
 CONFIG_RTE_LIBRTE_PMD_PCAP=y
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 0078dc9..efa0db7 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -241,6 +241,11 @@ CONFIG_RTE_PMD_RING_MAX_RX_RINGS=16
 CONFIG_RTE_PMD_RING_MAX_TX_RINGS=16
 
 #
+# Max pthread name len
+#
+CONFIG_RTE_MAX_THREAD_NAME_LEN=16
+
+#
 # Compile software PMD backed by PCAP files
 #
 CONFIG_RTE_LIBRTE_PMD_PCAP=n
diff --git a/examples/vhost/Makefile b/examples/vhost/Makefile
index c269466..e95c68a 100644
--- a/examples/vhost/Makefile
+++ b/examples/vhost/Makefile
@@ -52,6 +52,7 @@ SRCS-y := main.c
 
 CFLAGS += -O2 -D_FILE_OFFSET_BITS=64
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -D_GNU_SOURCE
 
 include $(RTE_SDK)/mk/rte.extapp.mk
 
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index ad10f82..d337e88 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -2891,6 +2891,7 @@ main(int argc, char *argv[])
 	uint8_t portid;
 	uint16_t queue_id;
 	static pthread_t tid;
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
@@ -3017,8 +3018,21 @@ main(int argc, char *argv[])
 	memset(&dev_statistics, 0, sizeof(dev_statistics));
 
 	/* Enable stats if the user option is set. */
-	if (enable_stats)
-		pthread_create(&tid, NULL, (void*)print_stats, NULL );
+	if (enable_stats) {
+		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "print-stats");
+
+		ret = pthread_create(&tid, NULL, (void*)print_stats, NULL );
+
+		if (ret != 0)
+			rte_exit(EXIT_FAILURE,
+				"Cannot create print-stats thread\n");
+
+		ret = pthread_setname_np(tid, thread_name);
+
+		if (ret != 0)
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"Cannot set print-stats name\n");
+	}
 
 	/* Launch all data cores. */
 	if (zero_copy == 0) {
diff --git a/examples/vhost_xen/Makefile b/examples/vhost_xen/Makefile
index e6fa1a1..47e1489 100644
--- a/examples/vhost_xen/Makefile
+++ b/examples/vhost_xen/Makefile
@@ -46,6 +46,7 @@ SRCS-y := main.c vhost_monitor.c xenstore_parse.c
 
 CFLAGS += -O2 -I/usr/local/include -D_FILE_OFFSET_BITS=64 -Wno-unused-parameter
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -D_GNU_SOURCE
 LDFLAGS += -lxenstore
 
 include $(RTE_SDK)/mk/rte.extapp.mk
diff --git a/examples/vhost_xen/main.c b/examples/vhost_xen/main.c
index b4a86e3..7bce482 100644
--- a/examples/vhost_xen/main.c
+++ b/examples/vhost_xen/main.c
@@ -1433,6 +1433,7 @@ main(int argc, char *argv[])
 	int ret;
 	uint8_t portid;
 	static pthread_t tid;
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	/* init EAL */
 	ret = rte_eal_init(argc, argv);
@@ -1505,8 +1506,23 @@ main(int argc, char *argv[])
 	memset(&dev_statistics, 0, sizeof(dev_statistics));
 
 	/* Enable stats if the user option is set. */
-	if (enable_stats)
-		pthread_create(&tid, NULL, (void*)print_stats, NULL );
+	if (enable_stats) {
+
+		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
+			"print-xen-stats");
+
+		ret = pthread_create(&tid, NULL, (void*)print_stats, NULL );
+
+		if (ret != 0)
+			rte_exit(EXIT_FAILURE,
+				"Cannot create print-stats thread\n");
+
+		ret = pthread_setname_np(tid, thread_name);
+
+		if (ret != 0)
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"Cannot set print-stats name\n");
+	}
 
 	/* Launch all data cores. */
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 871d5f4..4dc25a8 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -438,6 +438,7 @@ rte_eal_init(int argc, char **argv)
 	pthread_t thread_id;
 	static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	if (!rte_atomic32_test_and_set(&run_once))
 		return -1;
@@ -525,6 +526,9 @@ rte_eal_init(int argc, char **argv)
 
 	RTE_LCORE_FOREACH_SLAVE(i) {
 
+		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
+				"lcore-slave-%d", i);
+
 		/*
 		 * create communication pipes between master thread
 		 * and children
@@ -541,6 +545,9 @@ rte_eal_init(int argc, char **argv)
 				     eal_thread_loop, NULL);
 		if (ret != 0)
 			rte_panic("Cannot create thread\n");
+
+		pthread_set_name_np(lcore_config[i].thread_id,
+					(const char *)thread_name);
 	}
 
 	/*
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 01f7b70..c7aba70 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -93,6 +93,8 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_thread.c
 
 CFLAGS_eal.o := -D_GNU_SOURCE
 CFLAGS_eal_interrupts.o := -D_GNU_SOURCE
+CFLAGS_eal_pci_vfio_mp_sync.o := -D_GNU_SOURCE
+CFLAGS_eal_timer.o := -D_GNU_SOURCE
 CFLAGS_eal_lcore.o := -D_GNU_SOURCE
 CFLAGS_eal_thread.o := -D_GNU_SOURCE
 CFLAGS_eal_log.o := -D_GNU_SOURCE
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index bd770cf..058aed7 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -705,6 +705,7 @@ rte_eal_init(int argc, char **argv)
 	struct shared_driver *solib = NULL;
 	const char *logid;
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	if (!rte_atomic32_test_and_set(&run_once))
 		return -1;
@@ -816,6 +817,9 @@ rte_eal_init(int argc, char **argv)
 
 	RTE_LCORE_FOREACH_SLAVE(i) {
 
+		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
+			"lcore-slave-%d", i);
+
 		/*
 		 * create communication pipes between master thread
 		 * and children
@@ -832,6 +836,13 @@ rte_eal_init(int argc, char **argv)
 				     eal_thread_loop, NULL);
 		if (ret != 0)
 			rte_panic("Cannot create thread\n");
+
+		ret = pthread_setname_np(lcore_config[i].thread_id,
+						thread_name);
+
+		if (ret != 0)
+			RTE_LOG (ERR, EAL,
+				"Cannot set name for lcore thread\n");
 	}
 
 	/*
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 66deda2..a844435 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -66,6 +66,7 @@
 
 #include "eal_private.h"
 #include "eal_vfio.h"
+#include "eal_thread.h"
 
 #define EAL_INTR_EPOLL_WAIT_FOREVER (-1)
 
@@ -837,7 +838,8 @@ eal_intr_thread_main(__rte_unused void *arg)
 int
 rte_eal_intr_init(void)
 {
-	int ret = 0;
+	int ret = 0, ret_1 = 0;
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	/* init the global interrupt source head */
 	TAILQ_INIT(&intr_sources);
@@ -849,13 +851,25 @@ rte_eal_intr_init(void)
 	if (pipe(intr_pipe.pipefd) < 0)
 		return -1;
 
+	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "eal-intr-thread");
+
+
 	/* create the host thread to wait/handle the interrupt */
 	ret = pthread_create(&intr_thread, NULL,
 			eal_intr_thread_main, NULL);
-	if (ret != 0)
+	if (ret != 0) {
 		RTE_LOG(ERR, EAL,
 			"Failed to create thread for interrupt handling\n");
+	} else {
+		ret_1 = pthread_setname_np(intr_thread, thread_name);
+
+		/*
+		 * Log an error if setname fails and return rc of pthread_create.
+		 */
+		if (ret_1 != 0)
+			RTE_LOG(ERR, EAL,
+			"Failed to set thread name for interrupt handling\n");
 
+	}
 	return -ret;
 }
-
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c
index fec7080..c7ad972 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c
@@ -34,6 +34,7 @@
 #include <string.h>
 #include <fcntl.h>
 #include <sys/socket.h>
+#include <pthread.h>
 
 /* sys/un.h with __USE_MISC uses strlen, which is unsafe */
 #ifdef __USE_MISC
@@ -54,6 +55,7 @@
 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
+#include "eal_thread.h"
 
 /**
  * @file
@@ -374,20 +376,30 @@ int
 pci_vfio_mp_sync_setup(void)
 {
 	int ret;
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	if (vfio_mp_sync_socket_setup() < 0) {
 		RTE_LOG(ERR, EAL, "Failed to set up local socket!\n");
 		return -1;
 	}
 
+	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "pci-vfio-sync");
+
 	ret = pthread_create(&socket_thread, NULL,
 			pci_vfio_mp_sync_thread, NULL);
 	if (ret) {
-		RTE_LOG(ERR, EAL, "Failed to create thread for communication with "
-				"secondary processes!\n");
+		RTE_LOG(ERR, EAL,
+			"Failed to create thread for communication with secondary processes!\n");
 		close(mp_socket_fd);
 		return -1;
 	}
+
+	ret = pthread_setname_np(socket_thread, thread_name);
+
+	if (ret)
+		RTE_LOG(ERR, EAL,
+			"Failed to set thread name for secondary processes!\n");
+
 	return 0;
 }
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
index 169c6e1..8f85b2a 100644
--- a/lib/librte_eal/linuxapp/eal/eal_timer.c
+++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
@@ -186,6 +186,7 @@ int
 rte_eal_hpet_init(int make_default)
 {
 	int fd, ret;
+	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 
 	if (internal_config.no_hpet) {
 		RTE_LOG(INFO, EAL, "HPET is disabled\n");
@@ -224,16 +225,24 @@ rte_eal_hpet_init(int make_default)
 
 	eal_hpet_msb = (eal_hpet->counter_l >> 30);
 
+	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "hpet-msb-inc");
+
 	/* create a thread that will increment a global variable for
 	 * msb (hpet is 32 bits by default under linux) */
 	ret = pthread_create(&msb_inc_thread_id, NULL,
 			(void *(*)(void *))hpet_msb_inc, NULL);
-	if (ret < 0) {
+	if (ret != 0) {
 		RTE_LOG(ERR, EAL, "ERROR: Cannot create HPET timer thread!\n");
 		internal_config.no_hpet = 1;
 		return -1;
 	}
 
+	ret = pthread_setname_np(msb_inc_thread_id, thread_name);
+
+	if (ret != 0)
+		RTE_LOG(ERR, EAL,
+			"ERROR: Cannot set HPET timer thread name!\n");
+
 	if (make_default)
 		eal_timer_source = EAL_TIMER_HPET;
 	return 0;
-- 
1.9.1

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Stephen Hemminger]

On Wed, 22 Apr 2015 14:05:48 -0700
Ravi Kerur <rkerur@gmail.com> wrote:

> Add code to set names to threads via pthread APIs.
> In Linux corresponding _getname_ is available, however, FreeBSD
> doesn't have corresponding _get_name API available yet. Hence _getname_
> is not yet used in the code.
> 
> Ravi Kerur (1):
>   Use pthread_setname apis
> 
>  config/common_bsdapp                               |  5 +++++
>  config/common_linuxapp                             |  5 +++++
>  examples/vhost/Makefile                            |  1 +
>  examples/vhost/main.c                              | 18 ++++++++++++++++--
>  examples/vhost_xen/Makefile                        |  1 +
>  examples/vhost_xen/main.c                          | 20 ++++++++++++++++++--
>  lib/librte_eal/bsdapp/eal/eal.c                    |  7 +++++++
>  lib/librte_eal/linuxapp/eal/Makefile               |  2 ++
>  lib/librte_eal/linuxapp/eal/eal.c                  | 11 +++++++++++
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 20 +++++++++++++++++---
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c | 16 ++++++++++++++--
>  lib/librte_eal/linuxapp/eal/eal_timer.c            | 11 ++++++++++-
>  12 files changed, 107 insertions(+), 10 deletions(-)
> 

Since it possible to have multiple DPDK applications in same environment,
and the thread name size is so limited, I wonder if this is a good idea.

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Matthew Hall]

On Wed, Apr 22, 2015 at 03:57:44PM -0700, Stephen Hemminger wrote:
> Since it possible to have multiple DPDK applications in same environment,
> and the thread name size is so limited, I wonder if this is a good idea.

Why not try to opportunistically make the code easier to debug? DPDK is not 
always the easiest thing to debug, but at least it's way better than the 
kernel.

Matthew.

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Stephen Hemminger]

In our application we already use setname and have a policy for what the
names look like.
This won't help

On Wed, Apr 22, 2015 at 5:19 PM, Matthew Hall <mhall@mhcomputing.net> wrote:

> On Wed, Apr 22, 2015 at 03:57:44PM -0700, Stephen Hemminger wrote:
> > Since it possible to have multiple DPDK applications in same environment,
> > and the thread name size is so limited, I wonder if this is a good idea.
>
> Why not try to opportunistically make the code easier to debug? DPDK is not
> always the easiest thing to debug, but at least it's way better than the
> kernel.
>
> Matthew.
>

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Matthew Hall]

On Wed, Apr 22, 2015 at 05:39:40PM -0700, Stephen Hemminger wrote:
> In our application we already use setname and have a policy for what the
> names look like.
> This won't help

Not everybody does.

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Bruce Richardson]

On Wed, Apr 22, 2015 at 03:57:44PM -0700, Stephen Hemminger wrote:
> On Wed, 22 Apr 2015 14:05:48 -0700
> Ravi Kerur <rkerur@gmail.com> wrote:
> 
> > Add code to set names to threads via pthread APIs.
> > In Linux corresponding _getname_ is available, however, FreeBSD
> > doesn't have corresponding _get_name API available yet. Hence _getname_
> > is not yet used in the code.
> > 
> > Ravi Kerur (1):
> >   Use pthread_setname apis
> > 
> >  config/common_bsdapp                               |  5 +++++
> >  config/common_linuxapp                             |  5 +++++
> >  examples/vhost/Makefile                            |  1 +
> >  examples/vhost/main.c                              | 18 ++++++++++++++++--
> >  examples/vhost_xen/Makefile                        |  1 +
> >  examples/vhost_xen/main.c                          | 20 ++++++++++++++++++--
> >  lib/librte_eal/bsdapp/eal/eal.c                    |  7 +++++++
> >  lib/librte_eal/linuxapp/eal/Makefile               |  2 ++
> >  lib/librte_eal/linuxapp/eal/eal.c                  | 11 +++++++++++
> >  lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 20 +++++++++++++++++---
> >  lib/librte_eal/linuxapp/eal/eal_pci_vfio_mp_sync.c | 16 ++++++++++++++--
> >  lib/librte_eal/linuxapp/eal/eal_timer.c            | 11 ++++++++++-
> >  12 files changed, 107 insertions(+), 10 deletions(-)
> > 
> 
> Since it possible to have multiple DPDK applications in same environment,
> and the thread name size is so limited, I wonder if this is a good idea.

Can you be a bit more specific as to what your concern here is? This looks a good
idea to me.

/Bruce

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Thomas Monjalon]

Hi Ravi,
It seems to be a nice improvement but it needs some cleanup.

Checkpatch returns some errors.

2015-04-22 14:06, Ravi Kerur:
> use pthread_setname_np and pthread_set_name_np for Linux and
> FreeBSD respectively.
> Restrict pthread name len to 16 via config for both Linux and FreeBSD.

One of the most important part of the commit message is to answer why.
Here you probably should explain that the goal is to help debugging.

>  #
> +# Max pthread name len
> +#
> +CONFIG_RTE_MAX_THREAD_NAME_LEN=16

It doesn't have to be configurable. A define would be sufficient.

> +		snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "print-stats");

Why not put this line just before use of thread_name?

> +
> +		ret = pthread_create(&tid, NULL, (void*)print_stats, NULL );
> +
> +		if (ret != 0)
> +			rte_exit(EXIT_FAILURE,
> +				"Cannot create print-stats thread\n");
> +
> +		ret = pthread_setname_np(tid, thread_name);
> +
> +		if (ret != 0)
> +			RTE_LOG(ERR, VHOST_CONFIG,
> +				"Cannot set print-stats name\n");
[...]

> +		pthread_set_name_np(lcore_config[i].thread_id,
> +					(const char *)thread_name);

Is const casting really needed?

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Ravi Kerur]

On Sun, Jul 26, 2015 at 2:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> Hi Ravi,
> It seems to be a nice improvement but it needs some cleanup.
>
> Checkpatch returns some errors.
>
> 2015-04-22 14:06, Ravi Kerur:
> > use pthread_setname_np and pthread_set_name_np for Linux and
> > FreeBSD respectively.
> > Restrict pthread name len to 16 via config for both Linux and FreeBSD.
>
> One of the most important part of the commit message is to answer why.
> Here you probably should explain that the goal is to help debugging.
>

Sure will do it and will run checkpatch before next version.

>
> >  #
> > +# Max pthread name len
> > +#
> > +CONFIG_RTE_MAX_THREAD_NAME_LEN=16
>
> It doesn't have to be configurable. A define would be sufficient.
>

Had run into issues(reported by Bruce) as name_len = 32 worked fine for
FreeBSD but not for Linux, hence thought of making it configurable for
Linux/FreeBSD and be able to have different name len's. Found out that
there is also a definition PTHREAD_MAX_NAMELEN_NP, if it's available on
both Linux and FreeBSD  I will use this, else should I restrict name_len =
16?

>
> > +             snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
> "print-stats");
>
> Why not put this line just before use of thread_name?
>

Will do it.

>
> > +
> > +             ret = pthread_create(&tid, NULL, (void*)print_stats, NULL
> );
> > +
> > +             if (ret != 0)
> > +                     rte_exit(EXIT_FAILURE,
> > +                             "Cannot create print-stats thread\n");
> > +
> > +             ret = pthread_setname_np(tid, thread_name);
> > +
> > +             if (ret != 0)
> > +                     RTE_LOG(ERR, VHOST_CONFIG,
> > +                             "Cannot set print-stats name\n");
> [...]
>
> > +             pthread_set_name_np(lcore_config[i].thread_id,
> > +                                     (const char *)thread_name);
>
> Is const casting really needed?
>

Function signature has a (const char *) for second parameter, I will double
check and remove if not needed.

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Stephen Hemminger]

On Mon, 27 Jul 2015 13:40:08 -0700
Ravi Kerur <rkerur@gmail.com> wrote:

> On Sun, Jul 26, 2015 at 2:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > Hi Ravi,
> > It seems to be a nice improvement but it needs some cleanup.
> >
> > Checkpatch returns some errors.
> >
> > 2015-04-22 14:06, Ravi Kerur:
> > > use pthread_setname_np and pthread_set_name_np for Linux and
> > > FreeBSD respectively.
> > > Restrict pthread name len to 16 via config for both Linux and FreeBSD.
> >
> > One of the most important part of the commit message is to answer why.
> > Here you probably should explain that the goal is to help debugging.
> >
> 
> Sure will do it and will run checkpatch before next version.
> 
> >
> > >  #
> > > +# Max pthread name len
> > > +#
> > > +CONFIG_RTE_MAX_THREAD_NAME_LEN=16
> >
> > It doesn't have to be configurable. A define would be sufficient.
> >

Definitely should not be in the DPDK config.
It is property of system, which change some config parameter doesn't change.

Manpage implies it is fixed in glibc.

       The pthread_setname_np() function can be used to  set  a
       unique  name  for  a  thread,  which can be useful for debugging multi‐
       threaded applications.  The thread name  is  a  meaningful  C  language
       string, whose length is restricted to 16 characters, including the ter‐
       minating null byte ('\0').  

> Had run into issues(reported by Bruce) as name_len = 32 worked fine for
> FreeBSD but not for Linux, hence thought of making it configurable for
> Linux/FreeBSD and be able to have different name len's. Found out that
> there is also a definition PTHREAD_MAX_NAMELEN_NP, if it's available on
> both Linux and FreeBSD  I will use this, else should I restrict name_len =
> 16?

Not that I see on Linux
$ find /usr/include -type f | xargs grep PTHREAD_MAX_NAMELEN_NP



> >
> > > +             snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
> > "print-stats");
> >
> > Why not put this line just before use of thread_name?
> >
> 
> Will do it.
> 
> >
> > > +
> > > +             ret = pthread_create(&tid, NULL, (void*)print_stats, NULL
> > );
> > > +
> > > +             if (ret != 0)
> > > +                     rte_exit(EXIT_FAILURE,
> > > +                             "Cannot create print-stats thread\n");
> > > +
> > > +             ret = pthread_setname_np(tid, thread_name);
> > > +
> > > +             if (ret != 0)
> > > +                     RTE_LOG(ERR, VHOST_CONFIG,
> > > +                             "Cannot set print-stats name\n");
> > [...]
> >
> > > +             pthread_set_name_np(lcore_config[i].thread_id,
> > > +                                     (const char *)thread_name);
> >
> > Is const casting really needed?
> >
> 
> Function signature has a (const char *) for second parameter, I will double
> check and remove if not needed.

You can always pass a char * when function takes const char *

Re: [dpdk-dev] [PATCH] Use pthread_setname APIs

[Ravi Kerur]

On Mon, Jul 27, 2015 at 2:09 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Mon, 27 Jul 2015 13:40:08 -0700
> Ravi Kerur <rkerur@gmail.com> wrote:
>
> > On Sun, Jul 26, 2015 at 2:54 PM, Thomas Monjalon <
> thomas.monjalon@6wind.com>
> > wrote:
> >
> > > Hi Ravi,
> > > It seems to be a nice improvement but it needs some cleanup.
> > >
> > > Checkpatch returns some errors.
> > >
> > > 2015-04-22 14:06, Ravi Kerur:
> > > > use pthread_setname_np and pthread_set_name_np for Linux and
> > > > FreeBSD respectively.
> > > > Restrict pthread name len to 16 via config for both Linux and
> FreeBSD.
> > >
> > > One of the most important part of the commit message is to answer why.
> > > Here you probably should explain that the goal is to help debugging.
> > >
> >
> > Sure will do it and will run checkpatch before next version.
> >
> > >
> > > >  #
> > > > +# Max pthread name len
> > > > +#
> > > > +CONFIG_RTE_MAX_THREAD_NAME_LEN=16
> > >
> > > It doesn't have to be configurable. A define would be sufficient.
> > >
>
> Definitely should not be in the DPDK config.
> It is property of system, which change some config parameter doesn't
> change.
>
> Manpage implies it is fixed in glibc.
>
>        The pthread_setname_np() function can be used to  set  a
>        unique  name  for  a  thread,  which can be useful for debugging
> multi‐
>        threaded applications.  The thread name  is  a  meaningful  C
> language
>        string, whose length is restricted to 16 characters, including the
> ter‐
>        minating null byte ('\0').
>
> > Had run into issues(reported by Bruce) as name_len = 32 worked fine for
> > FreeBSD but not for Linux, hence thought of making it configurable for
> > Linux/FreeBSD and be able to have different name len's. Found out that
> > there is also a definition PTHREAD_MAX_NAMELEN_NP, if it's available on
> > both Linux and FreeBSD  I will use this, else should I restrict name_len
> =
> > 16?
>
> Not that I see on Linux
> $ find /usr/include -type f | xargs grep PTHREAD_MAX_NAMELEN_NP
>
>
That's correct on Linux and look like it's not defined for FreeBSD as
well.  FreeBSD man page doesn't mention anything about string length too.

>
>
> > >
> > > > +             snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN,
> > > "print-stats");
> > >
> > > Why not put this line just before use of thread_name?
> > >
> >
> > Will do it.
> >
> > >
> > > > +
> > > > +             ret = pthread_create(&tid, NULL, (void*)print_stats,
> NULL
> > > );
> > > > +
> > > > +             if (ret != 0)
> > > > +                     rte_exit(EXIT_FAILURE,
> > > > +                             "Cannot create print-stats thread\n");
> > > > +
> > > > +             ret = pthread_setname_np(tid, thread_name);
> > > > +
> > > > +             if (ret != 0)
> > > > +                     RTE_LOG(ERR, VHOST_CONFIG,
> > > > +                             "Cannot set print-stats name\n");
> > > [...]
> > >
> > > > +             pthread_set_name_np(lcore_config[i].thread_id,
> > > > +                                     (const char *)thread_name);
> > >
> > > Is const casting really needed?
> > >
> >
> > Function signature has a (const char *) for second parameter, I will
> double
> > check and remove if not needed.
>
> You can always pass a char * when function takes const char *
>

That's correct, will remove it.