* [dpdk-dev] [PATCH] Use pthread_setname APIs @ 2015-04-22 21:05 Ravi Kerur 2015-04-22 21:06 ` Ravi Kerur 2015-04-22 22:57 ` Stephen Hemminger 0 siblings, 2 replies; 11+ messages in thread From: Ravi Kerur @ 2015-04-22 21:05 UTC (permalink / raw) To: dev 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-04-22 21:05 [dpdk-dev] [PATCH] Use pthread_setname APIs Ravi Kerur @ 2015-04-22 21:06 ` Ravi Kerur 2015-07-26 21:54 ` Thomas Monjalon 2015-04-22 22:57 ` Stephen Hemminger 1 sibling, 1 reply; 11+ messages in thread From: Ravi Kerur @ 2015-04-22 21:06 UTC (permalink / raw) To: dev 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-04-22 21:06 ` Ravi Kerur @ 2015-07-26 21:54 ` Thomas Monjalon 2015-07-27 20:40 ` Ravi Kerur 0 siblings, 1 reply; 11+ messages in thread From: Thomas Monjalon @ 2015-07-26 21:54 UTC (permalink / raw) To: Ravi Kerur; +Cc: dev 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-07-26 21:54 ` Thomas Monjalon @ 2015-07-27 20:40 ` Ravi Kerur 2015-07-27 21:09 ` Stephen Hemminger 0 siblings, 1 reply; 11+ messages in thread From: Ravi Kerur @ 2015-07-27 20:40 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-07-27 20:40 ` Ravi Kerur @ 2015-07-27 21:09 ` Stephen Hemminger 2015-07-27 21:48 ` Ravi Kerur 0 siblings, 1 reply; 11+ messages in thread From: Stephen Hemminger @ 2015-07-27 21:09 UTC (permalink / raw) To: Ravi Kerur; +Cc: dev 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 * ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-07-27 21:09 ` Stephen Hemminger @ 2015-07-27 21:48 ` Ravi Kerur 0 siblings, 0 replies; 11+ messages in thread From: Ravi Kerur @ 2015-07-27 21:48 UTC (permalink / raw) To: Stephen Hemminger; +Cc: dev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-04-22 21:05 [dpdk-dev] [PATCH] Use pthread_setname APIs Ravi Kerur 2015-04-22 21:06 ` Ravi Kerur @ 2015-04-22 22:57 ` Stephen Hemminger 2015-04-23 0:19 ` Matthew Hall 2015-04-23 8:16 ` Bruce Richardson 1 sibling, 2 replies; 11+ messages in thread From: Stephen Hemminger @ 2015-04-22 22:57 UTC (permalink / raw) To: Ravi Kerur; +Cc: dev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-04-22 22:57 ` Stephen Hemminger @ 2015-04-23 0:19 ` Matthew Hall 2015-04-23 0:39 ` Stephen Hemminger 2015-04-23 8:16 ` Bruce Richardson 1 sibling, 1 reply; 11+ messages in thread From: Matthew Hall @ 2015-04-23 0:19 UTC (permalink / raw) To: Stephen Hemminger; +Cc: dev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-04-23 0:19 ` Matthew Hall @ 2015-04-23 0:39 ` Stephen Hemminger 2015-04-23 0:52 ` Matthew Hall 0 siblings, 1 reply; 11+ messages in thread From: Stephen Hemminger @ 2015-04-23 0:39 UTC (permalink / raw) To: Matthew Hall; +Cc: dev 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-04-23 0:39 ` Stephen Hemminger @ 2015-04-23 0:52 ` Matthew Hall 0 siblings, 0 replies; 11+ messages in thread From: Matthew Hall @ 2015-04-23 0:52 UTC (permalink / raw) To: Stephen Hemminger; +Cc: dev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] Use pthread_setname APIs 2015-04-22 22:57 ` Stephen Hemminger 2015-04-23 0:19 ` Matthew Hall @ 2015-04-23 8:16 ` Bruce Richardson 1 sibling, 0 replies; 11+ messages in thread From: Bruce Richardson @ 2015-04-23 8:16 UTC (permalink / raw) To: Stephen Hemminger; +Cc: dev 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-07-27 21:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-22 21:05 [dpdk-dev] [PATCH] Use pthread_setname APIs Ravi Kerur 2015-04-22 21:06 ` Ravi Kerur 2015-07-26 21:54 ` Thomas Monjalon 2015-07-27 20:40 ` Ravi Kerur 2015-07-27 21:09 ` Stephen Hemminger 2015-07-27 21:48 ` Ravi Kerur 2015-04-22 22:57 ` Stephen Hemminger 2015-04-23 0:19 ` Matthew Hall 2015-04-23 0:39 ` Stephen Hemminger 2015-04-23 0:52 ` Matthew Hall 2015-04-23 8:16 ` Bruce Richardson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).