[dpdk-dev] Use pthread_setname APIs
Commit Message
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(-)
Comments
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?
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.
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 *
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.
@@ -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
@@ -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
@@ -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
@@ -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) {
@@ -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
@@ -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) {
@@ -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);
}
/*
@@ -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
@@ -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");
}
/*
@@ -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;
}
-
@@ -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;
}
@@ -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;