Re: [dpdk-dev] [PATCH v4 3/9] eal: introduce thread init helper
From: Olivier Matz <hidden>
Date: 2020-06-30 09:37:44
Hi David, one minor comment below On Fri, Jun 26, 2020 at 04:47:30PM +0200, David Marchand wrote:
quoted hunk ↗ jump to hunk
Introduce a helper responsible for initialising the per thread context. We can then have a unified context for EAL and non-EAL threads and remove copy/paste'd OS-specific helpers. Per EAL thread CPU affinity setting is separated from the thread init. It is to accommodate with Windows EAL where CPU affinity is not set at the moment. Besides, having affinity set by the master lcore in FreeBSD and Linux will make it possible to detect errors rather than panic in the child thread. But the cleanup when such an event happens is left for later. Signed-off-by: David Marchand <redacted> --- Changes since v1: - rebased on master, removed Windows workarounds wrt gettid and traces support, --- lib/librte_eal/common/eal_common_thread.c | 51 +++++++++++++---------- lib/librte_eal/common/eal_thread.h | 8 ++-- lib/librte_eal/freebsd/eal.c | 14 ++++++- lib/librte_eal/freebsd/eal_thread.c | 32 +------------- lib/librte_eal/linux/eal.c | 15 ++++++- lib/librte_eal/linux/eal_thread.c | 32 +------------- lib/librte_eal/windows/eal.c | 3 +- lib/librte_eal/windows/eal_thread.c | 10 +---- 8 files changed, 66 insertions(+), 99 deletions(-)diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c index 280c64bb76..afb30236c5 100644 --- a/lib/librte_eal/common/eal_common_thread.c +++ b/lib/librte_eal/common/eal_common_thread.c@@ -71,20 +71,10 @@ eal_cpuset_socket_id(rte_cpuset_t *cpusetp) return socket_id; } -int -rte_thread_set_affinity(rte_cpuset_t *cpusetp) +static void +thread_update_affinity(rte_cpuset_t *cpusetp) { - int s; - unsigned lcore_id; - pthread_t tid; - - tid = pthread_self(); - - s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp); - if (s != 0) { - RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n"); - return -1; - } + unsigned int lcore_id = rte_lcore_id(); /* store socket_id in TLS for quick access */ RTE_PER_LCORE(_socket_id) =@@ -94,14 +84,24 @@ rte_thread_set_affinity(rte_cpuset_t *cpusetp) memmove(&RTE_PER_LCORE(_cpuset), cpusetp, sizeof(rte_cpuset_t)); - lcore_id = rte_lcore_id(); if (lcore_id != (unsigned)LCORE_ID_ANY) { /* EAL thread will update lcore_config */ lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id); memmove(&lcore_config[lcore_id].cpuset, cpusetp, sizeof(rte_cpuset_t)); } +} +int +rte_thread_set_affinity(rte_cpuset_t *cpusetp) +{ + if (pthread_setaffinity_np(pthread_self(), sizeof(rte_cpuset_t), + cpusetp) != 0) { + RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n"); + return -1; + } + + thread_update_affinity(cpusetp); return 0; }@@ -147,6 +147,19 @@ eal_thread_dump_affinity(char *str, unsigned size) return ret; } +void +rte_thread_init(unsigned int lcore_id, rte_cpuset_t *cpuset) +{ + /* set the lcore ID in per-lcore memory area */ + RTE_PER_LCORE(_lcore_id) = lcore_id; + + /* acquire system unique id */ + rte_gettid();
If I understand properly, rte_gettid() is now also called for control thread. I don't think this behavior change can break anything, but it may be good to highlight it in the commit log. also, there are 2 spaces before "*/"