[v4,3/9] eal: introduce thread init helper
diff mbox series

Message ID 20200626144736.11011-4-david.marchand@redhat.com
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series
  • Register non-EAL threads as lcore
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

David Marchand June 26, 2020, 2:47 p.m. UTC
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 <david.marchand@redhat.com>
---
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(-)

Comments

Olivier Matz June 30, 2020, 9:37 a.m. UTC | #1
Hi David,

one minor comment below

On Fri, Jun 26, 2020 at 04:47:30PM +0200, David Marchand wrote:
> 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 <david.marchand@redhat.com>
> ---
> 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 "*/"
David Marchand June 30, 2020, 12:04 p.m. UTC | #2
On Tue, Jun 30, 2020 at 11:37 AM Olivier Matz <olivier.matz@6wind.com> wrote:
> >
> > +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.

Control thread could not use recursive locks before, because this
rte_gettid() was missing.
Worth mentioning yes.

Patch
diff mbox series

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();
+
+	thread_update_affinity(cpuset);
+
+	__rte_trace_mem_per_thread_alloc();
+}
 
 struct rte_thread_ctrl_params {
 	void *(*start_routine)(void *);
@@ -154,16 +167,14 @@  struct rte_thread_ctrl_params {
 	pthread_barrier_t configured;
 };
 
-static void *rte_thread_init(void *arg)
+static void *ctrl_thread_init(void *arg)
 {
 	int ret;
-	rte_cpuset_t *cpuset = &internal_config.ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
 	void *(*start_routine)(void *) = params->start_routine;
 	void *routine_arg = params->arg;
 
-	/* Store cpuset in TLS for quick access */
-	memmove(&RTE_PER_LCORE(_cpuset), cpuset, sizeof(rte_cpuset_t));
+	rte_thread_init(rte_lcore_id(), &internal_config.ctrl_cpuset);
 
 	ret = pthread_barrier_wait(&params->configured);
 	if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
@@ -171,8 +182,6 @@  static void *rte_thread_init(void *arg)
 		free(params);
 	}
 
-	__rte_trace_mem_per_thread_alloc();
-
 	return start_routine(routine_arg);
 }
 
@@ -194,7 +203,7 @@  rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	pthread_barrier_init(&params->configured, NULL, 2);
 
-	ret = pthread_create(thread, attr, rte_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
diff --git a/lib/librte_eal/common/eal_thread.h b/lib/librte_eal/common/eal_thread.h
index b40ed249ed..da5e7c93ba 100644
--- a/lib/librte_eal/common/eal_thread.h
+++ b/lib/librte_eal/common/eal_thread.h
@@ -16,12 +16,14 @@ 
 __rte_noreturn void *eal_thread_loop(void *arg);
 
 /**
- * Init per-lcore info for master thread
+ * Init per-lcore info in current thread.
  *
  * @param lcore_id
- *   identifier of master lcore
+ *   identifier of lcore.
+ * @param cpuset
+ *   CPU affinity for this thread.
  */
-void eal_thread_init_master(unsigned lcore_id);
+void rte_thread_init(unsigned int lcore_id, rte_cpuset_t *cpuset);
 
 /**
  * Get the NUMA socket id from cpu id.
diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c
index 00fc66bf7f..13e5de006f 100644
--- a/lib/librte_eal/freebsd/eal.c
+++ b/lib/librte_eal/freebsd/eal.c
@@ -877,7 +877,14 @@  rte_eal_init(int argc, char **argv)
 
 	eal_check_mem_on_local_socket();
 
-	eal_thread_init_master(rte_config.master_lcore);
+	if (pthread_setaffinity_np(pthread_self(), sizeof(rte_cpuset_t),
+			&lcore_config[rte_config.master_lcore].cpuset) != 0) {
+		rte_eal_init_alert("Cannot set affinity");
+		rte_errno = EINVAL;
+		return -1;
+	}
+	rte_thread_init(rte_config.master_lcore,
+		&lcore_config[rte_config.master_lcore].cpuset);
 
 	ret = eal_thread_dump_affinity(cpuset, sizeof(cpuset));
 
@@ -908,6 +915,11 @@  rte_eal_init(int argc, char **argv)
 		snprintf(thread_name, sizeof(thread_name),
 				"lcore-slave-%d", i);
 		rte_thread_setname(lcore_config[i].thread_id, thread_name);
+
+		ret = pthread_setaffinity_np(lcore_config[i].thread_id,
+			sizeof(rte_cpuset_t), &lcore_config[i].cpuset);
+		if (ret != 0)
+			rte_panic("Cannot set affinity\n");
 	}
 
 	/*
diff --git a/lib/librte_eal/freebsd/eal_thread.c b/lib/librte_eal/freebsd/eal_thread.c
index 40676d9ef5..c1fb8eb2d8 100644
--- a/lib/librte_eal/freebsd/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal_thread.c
@@ -66,29 +66,6 @@  rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
 	return rc;
 }
 
-/* set affinity for current thread */
-static int
-eal_thread_set_affinity(void)
-{
-	unsigned lcore_id = rte_lcore_id();
-
-	/* acquire system unique id  */
-	rte_gettid();
-
-	/* update EAL thread core affinity */
-	return rte_thread_set_affinity(&lcore_config[lcore_id].cpuset);
-}
-
-void eal_thread_init_master(unsigned lcore_id)
-{
-	/* set the lcore ID in per-lcore memory area */
-	RTE_PER_LCORE(_lcore_id) = lcore_id;
-
-	/* set CPU affinity */
-	if (eal_thread_set_affinity() < 0)
-		rte_panic("cannot set affinity\n");
-}
-
 /* main loop of threads */
 __rte_noreturn void *
 eal_thread_loop(__rte_unused void *arg)
@@ -113,19 +90,12 @@  eal_thread_loop(__rte_unused void *arg)
 	m2s = lcore_config[lcore_id].pipe_master2slave[0];
 	s2m = lcore_config[lcore_id].pipe_slave2master[1];
 
-	/* set the lcore ID in per-lcore memory area */
-	RTE_PER_LCORE(_lcore_id) = lcore_id;
-
-	/* set CPU affinity */
-	if (eal_thread_set_affinity() < 0)
-		rte_panic("cannot set affinity\n");
+	rte_thread_init(lcore_id, &lcore_config[lcore_id].cpuset);
 
 	ret = eal_thread_dump_affinity(cpuset, sizeof(cpuset));
-
 	RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%p;cpuset=[%s%s])\n",
 		lcore_id, thread_id, cpuset, ret == 0 ? "" : "...");
 
-	__rte_trace_mem_per_thread_alloc();
 	rte_eal_trace_thread_lcore_ready(lcore_id, cpuset);
 
 	/* read on our pipe to get commands */
diff --git a/lib/librte_eal/linux/eal.c b/lib/librte_eal/linux/eal.c
index 28a8b78517..8894cea50a 100644
--- a/lib/librte_eal/linux/eal.c
+++ b/lib/librte_eal/linux/eal.c
@@ -1205,10 +1205,16 @@  rte_eal_init(int argc, char **argv)
 
 	eal_check_mem_on_local_socket();
 
-	eal_thread_init_master(rte_config.master_lcore);
+	if (pthread_setaffinity_np(pthread_self(), sizeof(rte_cpuset_t),
+			&lcore_config[rte_config.master_lcore].cpuset) != 0) {
+		rte_eal_init_alert("Cannot set affinity");
+		rte_errno = EINVAL;
+		return -1;
+	}
+	rte_thread_init(rte_config.master_lcore,
+		&lcore_config[rte_config.master_lcore].cpuset);
 
 	ret = eal_thread_dump_affinity(cpuset, sizeof(cpuset));
-
 	RTE_LOG(DEBUG, EAL, "Master lcore %u is ready (tid=%zx;cpuset=[%s%s])\n",
 		rte_config.master_lcore, (uintptr_t)thread_id, cpuset,
 		ret == 0 ? "" : "...");
@@ -1240,6 +1246,11 @@  rte_eal_init(int argc, char **argv)
 		if (ret != 0)
 			RTE_LOG(DEBUG, EAL,
 				"Cannot set name for lcore thread\n");
+
+		ret = pthread_setaffinity_np(lcore_config[i].thread_id,
+			sizeof(rte_cpuset_t), &lcore_config[i].cpuset);
+		if (ret != 0)
+			rte_panic("Cannot set affinity\n");
 	}
 
 	/*
diff --git a/lib/librte_eal/linux/eal_thread.c b/lib/librte_eal/linux/eal_thread.c
index a52ebef3a4..07aec0c44d 100644
--- a/lib/librte_eal/linux/eal_thread.c
+++ b/lib/librte_eal/linux/eal_thread.c
@@ -66,29 +66,6 @@  rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
 	return rc;
 }
 
-/* set affinity for current EAL thread */
-static int
-eal_thread_set_affinity(void)
-{
-	unsigned lcore_id = rte_lcore_id();
-
-	/* acquire system unique id  */
-	rte_gettid();
-
-	/* update EAL thread core affinity */
-	return rte_thread_set_affinity(&lcore_config[lcore_id].cpuset);
-}
-
-void eal_thread_init_master(unsigned lcore_id)
-{
-	/* set the lcore ID in per-lcore memory area */
-	RTE_PER_LCORE(_lcore_id) = lcore_id;
-
-	/* set CPU affinity */
-	if (eal_thread_set_affinity() < 0)
-		rte_panic("cannot set affinity\n");
-}
-
 /* main loop of threads */
 __rte_noreturn void *
 eal_thread_loop(__rte_unused void *arg)
@@ -113,19 +90,12 @@  eal_thread_loop(__rte_unused void *arg)
 	m2s = lcore_config[lcore_id].pipe_master2slave[0];
 	s2m = lcore_config[lcore_id].pipe_slave2master[1];
 
-	/* set the lcore ID in per-lcore memory area */
-	RTE_PER_LCORE(_lcore_id) = lcore_id;
-
-	/* set CPU affinity */
-	if (eal_thread_set_affinity() < 0)
-		rte_panic("cannot set affinity\n");
+	rte_thread_init(lcore_id, &lcore_config[lcore_id].cpuset);
 
 	ret = eal_thread_dump_affinity(cpuset, sizeof(cpuset));
-
 	RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%zx;cpuset=[%s%s])\n",
 		lcore_id, (uintptr_t)thread_id, cpuset, ret == 0 ? "" : "...");
 
-	__rte_trace_mem_per_thread_alloc();
 	rte_eal_trace_thread_lcore_ready(lcore_id, cpuset);
 
 	/* read on our pipe to get commands */
diff --git a/lib/librte_eal/windows/eal.c b/lib/librte_eal/windows/eal.c
index 97c8427c73..adfaa00275 100644
--- a/lib/librte_eal/windows/eal.c
+++ b/lib/librte_eal/windows/eal.c
@@ -367,7 +367,8 @@  rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	eal_thread_init_master(rte_config.master_lcore);
+	rte_thread_init(rte_config.master_lcore,
+		&lcore_config[rte_config.master_lcore].cpuset);
 
 	RTE_LCORE_FOREACH_SLAVE(i) {
 
diff --git a/lib/librte_eal/windows/eal_thread.c b/lib/librte_eal/windows/eal_thread.c
index f12a2ec6ad..4f01881240 100644
--- a/lib/librte_eal/windows/eal_thread.c
+++ b/lib/librte_eal/windows/eal_thread.c
@@ -53,13 +53,6 @@  rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int slave_id)
 	return 0;
 }
 
-void
-eal_thread_init_master(unsigned int lcore_id)
-{
-	/* set the lcore ID in per-lcore memory area */
-	RTE_PER_LCORE(_lcore_id) = lcore_id;
-}
-
 /* main loop of threads */
 void *
 eal_thread_loop(void *arg __rte_unused)
@@ -84,8 +77,7 @@  eal_thread_loop(void *arg __rte_unused)
 	m2s = lcore_config[lcore_id].pipe_master2slave[0];
 	s2m = lcore_config[lcore_id].pipe_slave2master[1];
 
-	/* set the lcore ID in per-lcore memory area */
-	RTE_PER_LCORE(_lcore_id) = lcore_id;
+	rte_thread_init(lcore_id, &lcore_config[lcore_id].cpuset);
 
 	RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%zx;cpuset=[%s])\n",
 		lcore_id, (uintptr_t)thread_id, cpuset);