[PATCH 0/3] eal: rte_ctrl_thread_create API replacement

[PATCH 0/3] eal: rte_ctrl_thread_create API replacement

[Tyler Retzlaff]

Remove rte_ctrl_thread_create API which exposes pthread_t and provide
new rte_control_thread_create API based on EALs rte_thread_t.

To limit compatibility regression risk and ease of removal of the
existing rte_ctrl_thread_create in the future duplicate most of the
existing implementation. The duplication will be removed when
rte_ctrl_thread_create is finally duplicated/removed.

The unit test for rte_ctrl_thread_create has been duplicated to exercise
both the old and the new API during transition, as with the API the
duplicate test will be removed when the rte_ctrl_thread_create API is
removed.

Tyler Retzlaff (3):
  eal: add rte control thread create API
  test: add rte control thread create API test
  eal: deprecate pthread control thread create API

 app/test/test_lcores.c               | 44 +++++++++++++++++
 doc/guides/rel_notes/deprecation.rst |  4 ++
 lib/eal/common/eal_common_thread.c   | 93 ++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h         | 29 +++++++++++
 lib/eal/version.map                  |  3 ++
 5 files changed, 165 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 29 ++++++++++++
 lib/eal/version.map                |  3 ++
 3 files changed, 117 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index c5d8b43..ca85c51 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void * (*start_routine)(void *);
+		rte_thread_func thread_func;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
@@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.start_routine;
+
+	if (0 != ctrl_thread_init(arg))
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.thread_func;
+
+	if (0 != ctrl_thread_init(arg))
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (!params)
+		return -ENOMEM;
+
+	params->u.thread_func = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL) {
+		ret = rte_thread_setname(thread->opaque_id, name);
+		if (ret < 0)
+			RTE_LOG(DEBUG, EAL,
+				"Cannot set name for ctrl thread\n");
+	}
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		/* Yield the CPU. Using sched_yield call requires maintaining
+		 * another implementation for Windows as sched_yield is not
+		 * supported on Windows.
+		 */
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index b9edf70..ffc6207 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -95,6 +95,35 @@ int rte_thread_create(rte_thread_t *thread_id,
 		rte_thread_func thread_func, void *arg);
 
 /**
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the dataplane and service
+ * lcores are then excluded. If setting the name of the thread fails,
+ * the error is ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread (max 16 characters including '\0').
+ * @param attr
+ *   Attributes for the new thread.
+ * @param start_routine
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7..8f9eeb9 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,9 @@ EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+
+	# added in 23.03
+	rte_control_thread_create;
 };
 
 INTERNAL {
-- 
1.8.3.1

[PATCH 2/3] test: add rte control thread create API test

[Tyler Retzlaff]

Duplicate the rte_ctrl_thread_create test adapted to use
rte_control_thread create to keep both apis under test until
rte_ctrl_thread_create is removed.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 app/test/test_lcores.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index a6bb412..517dc3c 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -8,6 +8,7 @@
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_lcore.h>
+#include <rte_thread.h>
 
 #include "test.h"
 
@@ -352,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
 	return NULL;
 }
 
+static uint32_t control_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = Thread_DONE;
+
+	return 0;
+}
+
 static int
 test_ctrl_thread(void)
 {
@@ -379,6 +392,34 @@ static void *ctrl_thread_loop(void *arg)
 }
 
 static int
+test_control_thread(void)
+{
+	rte_thread_t jid;
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = Thread_INIT;
+	if (rte_control_thread_create((rte_thread_t *)&t->id, "test_control_threads",
+					NULL, control_thread_loop, t) != 0)
+		return -1;
+
+	/* Wait till the control thread exits.
+	 * This also acts as the barrier such that the memory operations
+	 * in control thread are visible to this thread.
+	 */
+	jid.opaque_id = t->id;
+	rte_thread_join(jid, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != Thread_DONE)
+		return -1;
+
+	return 0;
+}
+
+static int
 test_lcores(void)
 {
 	unsigned int eal_threads_count = 0;
@@ -411,6 +452,9 @@ static void *ctrl_thread_loop(void *arg)
 	if (test_ctrl_thread() < 0)
 		return TEST_FAILED;
 
+	if (test_control_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
-- 
1.8.3.1

[PATCH 3/3] eal: deprecate pthread control thread create API

[Tyler Retzlaff]

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b9b02dc..cb74e08 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -119,3 +119,7 @@ Deprecation Notices
   Its removal has been postponed to let potential users report interest
   in maintaining it.
   In the absence of such interest, this library will be removed in DPDK 23.11.
+
+* eal: The function ``rte_ctrl_thread_create`` will be removed and
+  replaced by the new ``rte_control_thread_create``api, continuing the
+  effort to decouple eal from platform-specific thread implementations.
-- 
1.8.3.1

RE: [PATCH 0/3] eal: rte_ctrl_thread_create API replacement

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Monday, 5 December 2022 21.24
> 
> Remove rte_ctrl_thread_create API which exposes pthread_t and provide
> new rte_control_thread_create API based on EALs rte_thread_t.
> 
> To limit compatibility regression risk and ease of removal of the
> existing rte_ctrl_thread_create in the future duplicate most of the
> existing implementation. The duplication will be removed when
> rte_ctrl_thread_create is finally duplicated/removed.
> 
> The unit test for rte_ctrl_thread_create has been duplicated to
> exercise
> both the old and the new API during transition, as with the API the
> duplicate test will be removed when the rte_ctrl_thread_create API is
> removed.
> 
> Tyler Retzlaff (3):
>   eal: add rte control thread create API
>   test: add rte control thread create API test
>   eal: deprecate pthread control thread create API
> 
>  app/test/test_lcores.c               | 44 +++++++++++++++++
>  doc/guides/rel_notes/deprecation.rst |  4 ++
>  lib/eal/common/eal_common_thread.c   | 93
> ++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h         | 29 +++++++++++
>  lib/eal/version.map                  |  3 ++
>  5 files changed, 165 insertions(+), 8 deletions(-)
> 
> --
> 1.8.3.1
> 

Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 1/3] eal: add rte control thread create API

[Stephen Hemminger]

On Mon,  5 Dec 2022 12:24:26 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h       | 29 ++++++++++++
>  lib/eal/version.map                |  3 ++
>  3 files changed, 117 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index c5d8b43..ca85c51 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
>  };
>  
>  struct rte_thread_ctrl_params {
> -	void *(*start_routine)(void *);
> +	union {
> +		void * (*start_routine)(void *);
> +		rte_thread_func thread_func;
> +	} u;

Why not just use rte_thread_func, this in internal.

>  	void *arg;
>  	int ret;
>  	/* Control thread status.
> @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
>  	enum __rte_ctrl_thread_status ctrl_thread_status;
>  };
>  
> -static void *ctrl_thread_init(void *arg)
> +static int ctrl_thread_init(void *arg)
>  {
>  	struct internal_config *internal_conf =
>  		eal_get_internal_configuration();
>  	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>  	struct rte_thread_ctrl_params *params = arg;
> -	void *(*start_routine)(void *) = params->start_routine;
> -	void *routine_arg = params->arg;
>  
>  	__rte_thread_init(rte_lcore_id(), cpuset);
>  	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
> @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
>  	if (params->ret != 0) {
>  		__atomic_store_n(&params->ctrl_thread_status,
>  			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> -		return NULL;
> +		return params->ret;
>  	}
>  
>  	__atomic_store_n(&params->ctrl_thread_status,
>  		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
>  
> -	return start_routine(routine_arg);
> +	return 0;
> +}
> +
> +static void *ctrl_thread_start(void *arg)
> +{
> +	struct rte_thread_ctrl_params *params = arg;
> +	void *(*start_routine)(void *) = params->u.start_routine;
> +
> +	if (0 != ctrl_thread_init(arg))
> +		return NULL;

DPDK uses the Linux not Windows coding style.
Windows uses the constant on left side of comparison because of the common
programming error of putting assignment where conditional was intended.
Linux uses a compiler that puts out a warning, so the more natural
style is action != result

Re: [PATCH 3/3] eal: deprecate pthread control thread create API

[Stephen Hemminger]

On Mon,  5 Dec 2022 12:24:28 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> +
> +* eal: The function ``rte_ctrl_thread_create`` will be removed and
> +  replaced by the new ``rte_control_thread_create``api, continuing the
> +  effort to decouple eal from platform-specific thread implementations.

If you want to change this (which is a good idea)
then mark the function with __rte_deprecated now, and cleanup all the examples
and test programs please.

Re: [PATCH 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> On Mon,  5 Dec 2022 12:24:26 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> >  lib/eal/version.map                |  3 ++
> >  3 files changed, 117 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > index c5d8b43..ca85c51 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >  
> >  struct rte_thread_ctrl_params {
> > -	void *(*start_routine)(void *);
> > +	union {
> > +		void * (*start_routine)(void *);
> > +		rte_thread_func thread_func;
> > +	} u;
> 
> Why not just use rte_thread_func, this in internal.
> 
> >  	void *arg;
> >  	int ret;
> >  	/* Control thread status.
> > @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
> >  	enum __rte_ctrl_thread_status ctrl_thread_status;
> >  };
> >  
> > -static void *ctrl_thread_init(void *arg)
> > +static int ctrl_thread_init(void *arg)
> >  {
> >  	struct internal_config *internal_conf =
> >  		eal_get_internal_configuration();
> >  	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
> >  	struct rte_thread_ctrl_params *params = arg;
> > -	void *(*start_routine)(void *) = params->start_routine;
> > -	void *routine_arg = params->arg;
> >  
> >  	__rte_thread_init(rte_lcore_id(), cpuset);
> >  	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
> > @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
> >  	if (params->ret != 0) {
> >  		__atomic_store_n(&params->ctrl_thread_status,
> >  			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> > -		return NULL;
> > +		return params->ret;
> >  	}
> >  
> >  	__atomic_store_n(&params->ctrl_thread_status,
> >  		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
> >  
> > -	return start_routine(routine_arg);
> > +	return 0;
> > +}
> > +
> > +static void *ctrl_thread_start(void *arg)
> > +{
> > +	struct rte_thread_ctrl_params *params = arg;
> > +	void *(*start_routine)(void *) = params->u.start_routine;
> > +
> > +	if (0 != ctrl_thread_init(arg))
> > +		return NULL;
> 
> DPDK uses the Linux not Windows coding style.
> Windows uses the constant on left side of comparison because of the common
> programming error of putting assignment where conditional was intended.
> Linux uses a compiler that puts out a warning, so the more natural
> style is action != result

yes, of course.

i will fix this along with another minor style problem picked up by the
CI.

thanks.

Re: [PATCH 3/3] eal: deprecate pthread control thread create API

[Tyler Retzlaff]

On Mon, Dec 05, 2022 at 01:18:05PM -0800, Stephen Hemminger wrote:
> On Mon,  5 Dec 2022 12:24:28 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > +
> > +* eal: The function ``rte_ctrl_thread_create`` will be removed and
> > +  replaced by the new ``rte_control_thread_create``api, continuing the
> > +  effort to decouple eal from platform-specific thread implementations.
> 
> If you want to change this (which is a good idea)
> then mark the function with __rte_deprecated now, and cleanup all the examples
> and test programs please.

i would like to mark it deprecated now but it seems the policy governing
abi replacement prevent me from doing so.

```
3.3.3. New API replacing previous one
If a new API proposed functionally replaces an existing one, when the
new API becomes non-experimental then the old one is marked with
__rte_deprecated. Deprecated APIs are removed completely just after the
next LTS.
```

as i interpreted this i am not permitted to mark the old api
__rte_deprecated until the new api is no longer marked
__rte_experimental.

of course i'm happy to skip marking the new api __rte_experimental if
people find that a satisfactory solution?

let me know.

[PATCH v2 0/3] eal: rte_ctrl_thread_create API replacement

[Tyler Retzlaff]

Remove rte_ctrl_thread_create API which exposes pthread_t and provide
new rte_control_thread_create API based on EALs rte_thread_t.

To limit compatibility regression risk and ease of removal of the
existing rte_ctrl_thread_create in the future duplicate most of the
existing implementation. The duplication will be removed when
rte_ctrl_thread_create is finally duplicated/removed.

The unit test for rte_ctrl_thread_create has been duplicated to exercise
both the old and the new API during transition, as with the API the
duplicate test will be removed when the rte_ctrl_thread_create API is
removed.

v2:
  * correct style error void * (*foo) -> void *(*foo) 
  * place retval on lhs of comparison 0 != foo() -> foo() != 0
  * add missing commit description on patch 3/3
  * add cast uintptr_t to pthread_t where appropriate
  * fix doxygen @param names to match parameter names

Tyler Retzlaff (3):
  eal: add rte control thread create API
  test: add rte control thread create API test
  eal: deprecate pthread control thread create API

 app/test/test_lcores.c               | 44 +++++++++++++++++
 doc/guides/rel_notes/deprecation.rst |  4 ++
 lib/eal/common/eal_common_thread.c   | 93 ++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h         | 29 +++++++++++
 lib/eal/version.map                  |  3 ++
 5 files changed, 165 insertions(+), 8 deletions(-)

-- 
1.8.3.1

Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v2 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

Add rte_control_thread_create API as a replacement for
rte_ctrl_thread_create to allow deprecation of the use of platform
specific types in DPDK public API.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 29 ++++++++++++
 lib/eal/version.map                |  3 ++
 3 files changed, 117 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index c5d8b43..7950b50 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void *(*start_routine)(void *);
+		rte_thread_func thread_func;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
@@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.thread_func;
+
+	if (ctrl_thread_init(arg) != 0)
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (!params)
+		return -ENOMEM;
+
+	params->u.thread_func = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL) {
+		ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
+		if (ret < 0)
+			RTE_LOG(DEBUG, EAL,
+				"Cannot set name for ctrl thread\n");
+	}
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		/* Yield the CPU. Using sched_yield call requires maintaining
+		 * another implementation for Windows as sched_yield is not
+		 * supported on Windows.
+		 */
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index b9edf70..ae7afbe 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -95,6 +95,35 @@ int rte_thread_create(rte_thread_t *thread_id,
 		rte_thread_func thread_func, void *arg);
 
 /**
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the dataplane and service
+ * lcores are then excluded. If setting the name of the thread fails,
+ * the error is ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread (max 16 characters including '\0').
+ * @param thread_attr
+ *   Attributes for the new thread.
+ * @param thread_func
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7..8f9eeb9 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,9 @@ EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+
+	# added in 23.03
+	rte_control_thread_create;
 };
 
 INTERNAL {
-- 
1.8.3.1

[PATCH v2 2/3] test: add rte control thread create API test

[Tyler Retzlaff]

Duplicate the rte_ctrl_thread_create test adapted to use
rte_control_thread_create to keep both apis under test until
rte_ctrl_thread_create is removed.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 app/test/test_lcores.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index a6bb412..517dc3c 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -8,6 +8,7 @@
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_lcore.h>
+#include <rte_thread.h>
 
 #include "test.h"
 
@@ -352,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
 	return NULL;
 }
 
+static uint32_t control_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = Thread_DONE;
+
+	return 0;
+}
+
 static int
 test_ctrl_thread(void)
 {
@@ -379,6 +392,34 @@ static void *ctrl_thread_loop(void *arg)
 }
 
 static int
+test_control_thread(void)
+{
+	rte_thread_t jid;
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = Thread_INIT;
+	if (rte_control_thread_create((rte_thread_t *)&t->id, "test_control_threads",
+					NULL, control_thread_loop, t) != 0)
+		return -1;
+
+	/* Wait till the control thread exits.
+	 * This also acts as the barrier such that the memory operations
+	 * in control thread are visible to this thread.
+	 */
+	jid.opaque_id = t->id;
+	rte_thread_join(jid, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != Thread_DONE)
+		return -1;
+
+	return 0;
+}
+
+static int
 test_lcores(void)
 {
 	unsigned int eal_threads_count = 0;
@@ -411,6 +452,9 @@ static void *ctrl_thread_loop(void *arg)
 	if (test_ctrl_thread() < 0)
 		return TEST_FAILED;
 
+	if (test_control_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
-- 
1.8.3.1

[PATCH v2 3/3] eal: deprecate pthread control thread create API

[Tyler Retzlaff]

Give notice of rte_ctrl_thread_create deprecation.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b9b02dc..cb74e08 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -119,3 +119,7 @@ Deprecation Notices
   Its removal has been postponed to let potential users report interest
   in maintaining it.
   In the absence of such interest, this library will be removed in DPDK 23.11.
+
+* eal: The function ``rte_ctrl_thread_create`` will be removed and
+  replaced by the new ``rte_control_thread_create``api, continuing the
+  effort to decouple eal from platform-specific thread implementations.
-- 
1.8.3.1

Re: [PATCH 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> On Mon,  5 Dec 2022 12:24:26 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> >  lib/eal/version.map                |  3 ++
> >  3 files changed, 117 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > index c5d8b43..ca85c51 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >  
> >  struct rte_thread_ctrl_params {
> > -	void *(*start_routine)(void *);
> > +	union {
> > +		void * (*start_routine)(void *);
> > +		rte_thread_func thread_func;
> > +	} u;
> 
> Why not just use rte_thread_func, this in internal.

I'm not sure i completely understand your comment here. The main reason
for using a union is to avoid dealing with casting.  Later when the
rte_ctrl_thread_create is deprecated the union will be discarded.

No change was made in v2 but if you still think this should be addressed
let me know.

Re: [PATCH 1/3] eal: add rte control thread create API

[Stephen Hemminger]

On Tue, 6 Dec 2022 09:35:14 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> > On Mon,  5 Dec 2022 12:24:26 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >   
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> > >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> > >  lib/eal/version.map                |  3 ++
> > >  3 files changed, 117 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > > index c5d8b43..ca85c51 100644
> > > --- a/lib/eal/common/eal_common_thread.c
> > > +++ b/lib/eal/common/eal_common_thread.c
> > > @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> > >  };
> > >  
> > >  struct rte_thread_ctrl_params {
> > > -	void *(*start_routine)(void *);
> > > +	union {
> > > +		void * (*start_routine)(void *);
> > > +		rte_thread_func thread_func;
> > > +	} u;  
> > 
> > Why not just use rte_thread_func, this in internal.  
> 
> I'm not sure i completely understand your comment here. The main reason
> for using a union is to avoid dealing with casting.  Later when the
> rte_ctrl_thread_create is deprecated the union will be discarded.
> 
> No change was made in v2 but if you still think this should be addressed
> let me know.

Is it possible to do it without union or casting.
Aliasing function pointers is something that leads to long term issues.
The Linux kernel developers are actively fixing up all the casting there;
would like DPDK to adopt the same hygiene.

Re: [PATCH v2 1/3] eal: add rte control thread create API

[Mattias Rönnblom]

On 2022-12-06 18:28, Tyler Retzlaff wrote:
> Add rte_control_thread_create API as a replacement for
> rte_ctrl_thread_create to allow deprecation of the use of platform
> specific types in DPDK public API.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>   lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
>   lib/eal/include/rte_thread.h       | 29 ++++++++++++
>   lib/eal/version.map                |  3 ++
>   3 files changed, 117 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index c5d8b43..7950b50 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
>   };
>   
>   struct rte_thread_ctrl_params {
> -	void *(*start_routine)(void *);
> +	union {
> +		void *(*start_routine)(void *);
> +		rte_thread_func thread_func;

To be consistent with function naming scheme, where "ctrl" is the old 
API, and "control" the new, you could call the start functions something 
with "ctrl" and "control" as well.

Maybe it's worth mentioning that fact somewhere in the beginning of the 
file, as a comment (i.e., that "ctrl" denotes the old API).

> +	} u;
>   	void *arg;
>   	int ret;

Why is 'ret' needed? (This question is unrelated to your patch.)

>   	/* Control thread status.
> @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
>   	enum __rte_ctrl_thread_status ctrl_thread_status;
>   };
>   
> -static void *ctrl_thread_init(void *arg)
> +static int ctrl_thread_init(void *arg)
>   {
>   	struct internal_config *internal_conf =
>   		eal_get_internal_configuration();
>   	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>   	struct rte_thread_ctrl_params *params = arg;
> -	void *(*start_routine)(void *) = params->start_routine;
> -	void *routine_arg = params->arg;
>   
>   	__rte_thread_init(rte_lcore_id(), cpuset);
>   	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
> @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
>   	if (params->ret != 0) {
>   		__atomic_store_n(&params->ctrl_thread_status,
>   			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> -		return NULL;
> +		return params->ret;
>   	}
>   
>   	__atomic_store_n(&params->ctrl_thread_status,
>   		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
>   
> -	return start_routine(routine_arg);
> +	return 0;
> +}
> +
> +static void *ctrl_thread_start(void *arg)
> +{
> +	struct rte_thread_ctrl_params *params = arg;
> +	void *(*start_routine)(void *) = params->u.start_routine;
> +
> +	if (ctrl_thread_init(arg) != 0)
> +		return NULL;
> +
> +	return start_routine(params->arg);
> +}
> +
> +static uint32_t control_thread_start(void *arg)
> +{
> +	struct rte_thread_ctrl_params *params = arg;
> +	rte_thread_func start_routine = params->u.thread_func;
> +
> +	if (ctrl_thread_init(arg) != 0)
> +		return params->ret;
> +
> +	return start_routine(params->arg);
>   }
>   
>   int
> @@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
>   	if (!params)
>   		return -ENOMEM;
>   
> -	params->start_routine = start_routine;
> +	params->u.start_routine = start_routine;
>   	params->arg = arg;
>   	params->ret = 0;
>   	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>   
> -	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> +	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
>   	if (ret != 0) {
>   		free(params);
>   		return -ret;
> @@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
>   }
>   
>   int
> +rte_control_thread_create(rte_thread_t *thread, const char *name,
> +	const rte_thread_attr_t *attr,
> +	rte_thread_func start_routine, void *arg)
> +{
> +	struct rte_thread_ctrl_params *params;
> +	enum __rte_ctrl_thread_status ctrl_thread_status;
> +	int ret;
> +
> +	params = malloc(sizeof(*params));
> +	if (!params)

params == NULL

> +		return -ENOMEM;
> +
> +	params->u.thread_func = start_routine;
> +	params->arg = arg;
> +	params->ret = 0;
> +	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> +
> +	ret = rte_thread_create(thread, attr, control_thread_start, params);
> +	if (ret != 0) {
> +		free(params);
> +		return -ret;
> +	}
> +
> +	if (name != NULL) {
> +		ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
> +		if (ret < 0)
> +			RTE_LOG(DEBUG, EAL,
> +				"Cannot set name for ctrl thread\n");
> +	}
> +
> +	/* Wait for the control thread to initialize successfully */
> +	while ((ctrl_thread_status =
> +			__atomic_load_n(&params->ctrl_thread_status,
> +			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> +		/* Yield the CPU. Using sched_yield call requires maintaining
> +		 * another implementation for Windows as sched_yield is not
> +		 * supported on Windows.
> +		 */

sched_yield() also doesn't necessarily yield the CPU.

> +		rte_delay_us_sleep(1);
> +	}
> +
> +	/* Check if the control thread encountered an error */
> +	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
> +		/* ctrl thread is exiting */
> +		rte_thread_join(*thread, NULL);
> +	}
> +
> +	ret = params->ret;
> +	free(params);
> +
> +	return ret;
> +}
> +
> +int
>   rte_thread_register(void)
>   {
>   	unsigned int lcore_id;
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index b9edf70..ae7afbe 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -95,6 +95,35 @@ int rte_thread_create(rte_thread_t *thread_id,
>   		rte_thread_func thread_func, void *arg);
>   
>   /**
> + * Create a control thread.
> + *
> + * Creates a control thread with the given name and attributes. The
> + * affinity of the new thread is based on the CPU affinity retrieved
> + * at the time rte_eal_init() was called, the dataplane and service
> + * lcores are then excluded. If setting the name of the thread fails,

"the EAL threads are then excluded"

> + * the error is ignored and a debug message is logged.
> + *
> + * @param thread
> + *   Filled with the thread id of the new created thread.
> + * @param name
> + *   The name of the control thread (max 16 characters including '\0').

Is there a constant for this limit?

> + * @param thread_attr
> + *   Attributes for the new thread.
> + * @param thread_func
> + *   Function to be executed by the new thread.
> + * @param arg
> + *   Argument passed to start_routine.
> + * @return
> + *   On success, returns 0; on error, it returns a negative value
> + *   corresponding to the error number.

List the possible error codes.

> + */
> +__rte_experimental
> +int
> +rte_control_thread_create(rte_thread_t *thread, const char *name,
> +		const rte_thread_attr_t *thread_attr,
> +		rte_thread_func thread_func, void *arg);
> +
> +/**
>    * @warning
>    * @b EXPERIMENTAL: this API may change without prior notice.
>    *
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index 7ad12a7..8f9eeb9 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -440,6 +440,9 @@ EXPERIMENTAL {
>   	rte_thread_detach;
>   	rte_thread_equal;
>   	rte_thread_join;
> +
> +	# added in 23.03
> +	rte_control_thread_create;
>   };
>   
>   INTERNAL {

Re: [PATCH v2 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

On Wed, Dec 07, 2022 at 10:13:39AM +0100, Mattias Rönnblom wrote:
> On 2022-12-06 18:28, Tyler Retzlaff wrote:
> >Add rte_control_thread_create API as a replacement for
> >rte_ctrl_thread_create to allow deprecation of the use of platform
> >specific types in DPDK public API.
> >
> >Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >---
> >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> >  lib/eal/version.map                |  3 ++
> >  3 files changed, 117 insertions(+), 8 deletions(-)
> >
> >diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> >index c5d8b43..7950b50 100644
> >--- a/lib/eal/common/eal_common_thread.c
> >+++ b/lib/eal/common/eal_common_thread.c
> >@@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >  struct rte_thread_ctrl_params {
> >-	void *(*start_routine)(void *);
> >+	union {
> >+		void *(*start_routine)(void *);
> >+		rte_thread_func thread_func;
> 
> To be consistent with function naming scheme, where "ctrl" is the
> old API, and "control" the new, you could call the start functions
> something with "ctrl" and "control" as well.

i'll make this change in v3.

> 
> Maybe it's worth mentioning that fact somewhere in the beginning of
> the file, as a comment (i.e., that "ctrl" denotes the old API).

i'll make this change in v3.

> 
> >+	} u;
> >  	void *arg;
> >  	int ret;
> 
> Why is 'ret' needed? (This question is unrelated to your patch.)

i'm not the original author so difficult to answer authoritatively. but
if i have to speculate i'd say it might be to work around the windows
pthread_join stub being implemented as a noop. specifically it didn't
communicate the return value from the start_routine.

the recently added rte_thread_join addresses this (which
rte_control_thread_create uses) and could remove ret parameter and to
avoid touching the new function implementation in the future it can not
use ret now.

i'll make this change in v3.

for the original function i will leave the code as is to minimize the
diff. when rte_ctrl_thread_create is removed i'll make a note to
eliminate the ret parameter as well.

> 
> >  	/* Control thread status.
> >@@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
> >  	enum __rte_ctrl_thread_status ctrl_thread_status;
> >  };
> >-static void *ctrl_thread_init(void *arg)
> >+static int ctrl_thread_init(void *arg)
> >  {
> >  	struct internal_config *internal_conf =
> >  		eal_get_internal_configuration();
> >  	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
> >  	struct rte_thread_ctrl_params *params = arg;
> >-	void *(*start_routine)(void *) = params->start_routine;
> >-	void *routine_arg = params->arg;
> >  	__rte_thread_init(rte_lcore_id(), cpuset);
> >  	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
> >@@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
> >  	if (params->ret != 0) {
> >  		__atomic_store_n(&params->ctrl_thread_status,
> >  			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> >-		return NULL;
> >+		return params->ret;
> >  	}
> >  	__atomic_store_n(&params->ctrl_thread_status,
> >  		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
> >-	return start_routine(routine_arg);
> >+	return 0;
> >+}
> >+
> >+static void *ctrl_thread_start(void *arg)
> >+{
> >+	struct rte_thread_ctrl_params *params = arg;
> >+	void *(*start_routine)(void *) = params->u.start_routine;
> >+
> >+	if (ctrl_thread_init(arg) != 0)
> >+		return NULL;
> >+
> >+	return start_routine(params->arg);
> >+}
> >+
> >+static uint32_t control_thread_start(void *arg)
> >+{
> >+	struct rte_thread_ctrl_params *params = arg;
> >+	rte_thread_func start_routine = params->u.thread_func;
> >+
> >+	if (ctrl_thread_init(arg) != 0)
> >+		return params->ret;
> >+
> >+	return start_routine(params->arg);
> >  }
> >  int
> >@@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
> >  	if (!params)
> >  		return -ENOMEM;
> >-	params->start_routine = start_routine;
> >+	params->u.start_routine = start_routine;
> >  	params->arg = arg;
> >  	params->ret = 0;
> >  	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> >-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> >+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
> >  	if (ret != 0) {
> >  		free(params);
> >  		return -ret;
> >@@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
> >  }
> >  int
> >+rte_control_thread_create(rte_thread_t *thread, const char *name,
> >+	const rte_thread_attr_t *attr,
> >+	rte_thread_func start_routine, void *arg)
> >+{
> >+	struct rte_thread_ctrl_params *params;
> >+	enum __rte_ctrl_thread_status ctrl_thread_status;
> >+	int ret;
> >+
> >+	params = malloc(sizeof(*params));
> >+	if (!params)
> 
> params == NULL

copied from original code, i'll fix the style in the new function to
comply with the dpdk coding standard.

i'll fix in v3.

> 
> >+		return -ENOMEM;
> >+
> >+	params->u.thread_func = start_routine;
> >+	params->arg = arg;
> >+	params->ret = 0;
> >+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> >+
> >+	ret = rte_thread_create(thread, attr, control_thread_start, params);
> >+	if (ret != 0) {
> >+		free(params);
> >+		return -ret;
> >+	}
> >+
> >+	if (name != NULL) {
> >+		ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
> >+		if (ret < 0)
> >+			RTE_LOG(DEBUG, EAL,
> >+				"Cannot set name for ctrl thread\n");
> >+	}
> >+
> >+	/* Wait for the control thread to initialize successfully */
> >+	while ((ctrl_thread_status =
> >+			__atomic_load_n(&params->ctrl_thread_status,
> >+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> >+		/* Yield the CPU. Using sched_yield call requires maintaining
> >+		 * another implementation for Windows as sched_yield is not
> >+		 * supported on Windows.
> >+		 */
> 
> sched_yield() also doesn't necessarily yield the CPU.

copied from original code and understood, but are you requesting a
change here or just a comment correction? can you offer wording you
would find suitable?

> 
> >+		rte_delay_us_sleep(1);
> >+	}
> >+
> >+	/* Check if the control thread encountered an error */
> >+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
> >+		/* ctrl thread is exiting */
> >+		rte_thread_join(*thread, NULL);
> >+	}
> >+
> >+	ret = params->ret;
> >+	free(params);
> >+
> >+	return ret;
> >+}
> >+
> >+int
> >  rte_thread_register(void)
> >  {
> >  	unsigned int lcore_id;
> >diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> >index b9edf70..ae7afbe 100644
> >--- a/lib/eal/include/rte_thread.h
> >+++ b/lib/eal/include/rte_thread.h
> >@@ -95,6 +95,35 @@ int rte_thread_create(rte_thread_t *thread_id,
> >  		rte_thread_func thread_func, void *arg);
> >  /**
> >+ * Create a control thread.
> >+ *
> >+ * Creates a control thread with the given name and attributes. The
> >+ * affinity of the new thread is based on the CPU affinity retrieved
> >+ * at the time rte_eal_init() was called, the dataplane and service
> >+ * lcores are then excluded. If setting the name of the thread fails,
> 
> "the EAL threads are then excluded"

i'll modify in v3.

> 
> >+ * the error is ignored and a debug message is logged.
> >+ *
> >+ * @param thread
> >+ *   Filled with the thread id of the new created thread.
> >+ * @param name
> >+ *   The name of the control thread (max 16 characters including '\0').
> 
> Is there a constant for this limit?

i have a series introducing rte_lcore_{set,get}_name api that introduces
a constant (probably i'll post it today). as of this series there is no
constant.

> 
> >+ * @param thread_attr
> >+ *   Attributes for the new thread.
> >+ * @param thread_func
> >+ *   Function to be executed by the new thread.
> >+ * @param arg
> >+ *   Argument passed to start_routine.
> >+ * @return
> >+ *   On success, returns 0; on error, it returns a negative value
> >+ *   corresponding to the error number.
> 
> List the possible error codes.

i would like to see the range of codes be part of the api & abi i've
received resistance to the idea. for now i'll nack this comment which
leaves it consistent with other apis documentation.
> 
> >+ */
> >+__rte_experimental
> >+int
> >+rte_control_thread_create(rte_thread_t *thread, const char *name,
> >+		const rte_thread_attr_t *thread_attr,
> >+		rte_thread_func thread_func, void *arg);
> >+
> >+/**
> >   * @warning
> >   * @b EXPERIMENTAL: this API may change without prior notice.
> >   *
> >diff --git a/lib/eal/version.map b/lib/eal/version.map
> >index 7ad12a7..8f9eeb9 100644
> >--- a/lib/eal/version.map
> >+++ b/lib/eal/version.map
> >@@ -440,6 +440,9 @@ EXPERIMENTAL {
> >  	rte_thread_detach;
> >  	rte_thread_equal;
> >  	rte_thread_join;
> >+
> >+	# added in 23.03
> >+	rte_control_thread_create;
> >  };
> >  INTERNAL {

[PATCH v3 0/3] eal: rte_ctrl_thread_create API replacement

[Tyler Retzlaff]

Remove rte_ctrl_thread_create API which exposes pthread_t and provide
new rte_control_thread_create API based on EALs rte_thread_t.

To limit compatibility regression risk and ease of removal of the
existing rte_ctrl_thread_create in the future duplicate most of the
existing implementation. The duplication will be removed when
rte_ctrl_thread_create is finally duplicated/removed.

The unit test for rte_ctrl_thread_create has been duplicated to exercise
both the old and the new API during transition, as with the API the
duplicate test will be removed when the rte_ctrl_thread_create API is
removed.

v3:
  * use {ctrl,control}_start_routine for start_routine field names
  * fix conditional evaluation style p == NULL instead of !p
  * tweak documentation comment for rte_control_thread_create
    - "the EAL threads are then excluded"
    - note RTE_MAX_THREAD_NAME_LEN preprocessor definition
      is the name size (including terminating NUL) limit
  * add missing cast to uintptr_t

v2:
  * correct style error void * (*foo) -> void *(*foo)
  * place retval on lhs of comparison 0 != foo() -> foo() != 0
  * add missing commit description on patch 3/3
  * add cast uintptr_t to pthread_t where appropriate
  * fix doxygen @param names to match parameter names

Tyler Retzlaff (3):
  eal: add rte control thread create API
  test: add rte control thread create API test
  eal: deprecate pthread control thread create API

 app/test/test_lcores.c               | 42 ++++++++++++++++
 doc/guides/rel_notes/deprecation.rst |  4 ++
 lib/eal/common/eal_common_thread.c   | 93 ++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h         | 30 ++++++++++++
 lib/eal/version.map                  |  3 ++
 5 files changed, 164 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

Add rte_control_thread_create API as a replacement for
rte_ctrl_thread_create to allow deprecation of the use of platform
specific types in DPDK public API.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 30 ++++++++++++
 lib/eal/version.map                |  3 ++
 3 files changed, 118 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index c5d8b43..7c2552d 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void *(*ctrl_start_routine)(void *);
+		rte_thread_func control_start_routine;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
@@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.ctrl_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.control_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.ctrl_start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (params == NULL)
+		return -ENOMEM;
+
+	params->u.control_start_routine = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL) {
+		ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
+		if (ret < 0)
+			RTE_LOG(DEBUG, EAL,
+				"Cannot set name for ctrl thread\n");
+	}
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		/* Yield the CPU. Using sched_yield call requires maintaining
+		 * another implementation for Windows as sched_yield is not
+		 * supported on Windows.
+		 */
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index b9edf70..97c27a9 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -95,6 +95,36 @@ int rte_thread_create(rte_thread_t *thread_id,
 		rte_thread_func thread_func, void *arg);
 
 /**
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the EAL threads are then
+ * excluded. If setting the name of the thread fails, the error is
+ * ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread
+ *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
+ * @param thread_attr
+ *   Attributes for the new thread.
+ * @param thread_func
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7..8f9eeb9 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,9 @@ EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+
+	# added in 23.03
+	rte_control_thread_create;
 };
 
 INTERNAL {
-- 
1.8.3.1

[PATCH v3 2/3] test: add rte control thread create API test

[Tyler Retzlaff]

Duplicate the rte_ctrl_thread_create test adapted to use
rte_control_thread create to keep both apis under test until
rte_ctrl_thread_create is removed.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 app/test/test_lcores.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index a6bb412..375ecdb 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -8,6 +8,7 @@
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_lcore.h>
+#include <rte_thread.h>
 
 #include "test.h"
 
@@ -352,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
 	return NULL;
 }
 
+static uint32_t control_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = Thread_DONE;
+
+	return 0;
+}
+
 static int
 test_ctrl_thread(void)
 {
@@ -379,6 +392,32 @@ static void *ctrl_thread_loop(void *arg)
 }
 
 static int
+test_control_thread(void)
+{
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = Thread_INIT;
+	if (rte_control_thread_create((rte_thread_t *)&t->id, "test_control_threads",
+					NULL, control_thread_loop, t) != 0)
+		return -1;
+
+	/* Wait till the control thread exits.
+	 * This also acts as the barrier such that the memory operations
+	 * in control thread are visible to this thread.
+	 */
+	rte_thread_join((rte_thread_t){(uintptr_t)t->id}, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != Thread_DONE)
+		return -1;
+
+	return 0;
+}
+
+static int
 test_lcores(void)
 {
 	unsigned int eal_threads_count = 0;
@@ -411,6 +450,9 @@ static void *ctrl_thread_loop(void *arg)
 	if (test_ctrl_thread() < 0)
 		return TEST_FAILED;
 
+	if (test_control_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
-- 
1.8.3.1

[PATCH v3 3/3] eal: deprecate pthread control thread create API

[Tyler Retzlaff]

Give notice of rte_ctrl_thread_create deprecation.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b9b02dc..cb74e08 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -119,3 +119,7 @@ Deprecation Notices
   Its removal has been postponed to let potential users report interest
   in maintaining it.
   In the absence of such interest, this library will be removed in DPDK 23.11.
+
+* eal: The function ``rte_ctrl_thread_create`` will be removed and
+  replaced by the new ``rte_control_thread_create``api, continuing the
+  effort to decouple eal from platform-specific thread implementations.
-- 
1.8.3.1

Re: [PATCH v2 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

just following up.

On Wed, Dec 07, 2022 at 08:38:39AM -0800, Tyler Retzlaff wrote:
> On Wed, Dec 07, 2022 at 10:13:39AM +0100, Mattias Rönnblom wrote:
> > 
> > To be consistent with function naming scheme, where "ctrl" is the
> > old API, and "control" the new, you could call the start functions
> > something with "ctrl" and "control" as well.
> 
> i'll make this change in v3.
> 
> > 
> > Maybe it's worth mentioning that fact somewhere in the beginning of
> > the file, as a comment (i.e., that "ctrl" denotes the old API).
> 
> i'll make this change in v3.

didn't end up doing this, didn't think it was necessary once i looked at
it the commit history and the rename you suggested above should be
satisfactory.

> 
> > 
> > >+	} u;
> > >  	void *arg;
> > >  	int ret;
> > 
> > Why is 'ret' needed? (This question is unrelated to your patch.)
> 
> i'm not the original author so difficult to answer authoritatively. but
> if i have to speculate i'd say it might be to work around the windows
> pthread_join stub being implemented as a noop. specifically it didn't
> communicate the return value from the start_routine.
> 
> the recently added rte_thread_join addresses this (which
> rte_control_thread_create uses) and could remove ret parameter and to
> avoid touching the new function implementation in the future it can not
> use ret now.
> 
> i'll make this change in v3.

also did not do this, once looked at i was concerned about inter-mixing
too much semantic change in the implementation of a couple of the
functions to accomplish it. it is best revisited when the old api is
removed.

> > >+ * the error is ignored and a debug message is logged.
> > >+ *
> > >+ * @param thread
> > >+ *   Filled with the thread id of the new created thread.
> > >+ * @param name
> > >+ *   The name of the control thread (max 16 characters including '\0').
> > 
> > Is there a constant for this limit?
> 
> i have a series introducing rte_lcore_{set,get}_name api that introduces
> a constant (probably i'll post it today). as of this series there is no
> constant.

change in v3, i forgot we were talking about the thread length limit and
not the lcore name length limit and there was already a preprocessor
definition for the limit. api documentation comment was updated in v3.

thanks!

Re: [PATCH v2 1/3] eal: add rte control thread create API

[Mattias Rönnblom]

On 2022-12-07 17:38, Tyler Retzlaff wrote:
> On Wed, Dec 07, 2022 at 10:13:39AM +0100, Mattias Rönnblom wrote:
>> On 2022-12-06 18:28, Tyler Retzlaff wrote:
>>> Add rte_control_thread_create API as a replacement for
>>> rte_ctrl_thread_create to allow deprecation of the use of platform
>>> specific types in DPDK public API.
>>>
>>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>>> ---
>>>   lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
>>>   lib/eal/include/rte_thread.h       | 29 ++++++++++++
>>>   lib/eal/version.map                |  3 ++
>>>   3 files changed, 117 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
>>> index c5d8b43..7950b50 100644
>>> --- a/lib/eal/common/eal_common_thread.c
>>> +++ b/lib/eal/common/eal_common_thread.c
>>> @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
>>>   };
>>>   struct rte_thread_ctrl_params {
>>> -	void *(*start_routine)(void *);
>>> +	union {
>>> +		void *(*start_routine)(void *);
>>> +		rte_thread_func thread_func;
>>
>> To be consistent with function naming scheme, where "ctrl" is the
>> old API, and "control" the new, you could call the start functions
>> something with "ctrl" and "control" as well.
> 
> i'll make this change in v3.
> 
>>
>> Maybe it's worth mentioning that fact somewhere in the beginning of
>> the file, as a comment (i.e., that "ctrl" denotes the old API).
> 
> i'll make this change in v3.
> 
>>
>>> +	} u;
>>>   	void *arg;
>>>   	int ret;
>>
>> Why is 'ret' needed? (This question is unrelated to your patch.)
> 
> i'm not the original author so difficult to answer authoritatively. but
> if i have to speculate i'd say it might be to work around the windows
> pthread_join stub being implemented as a noop. specifically it didn't
> communicate the return value from the start_routine.
> 
> the recently added rte_thread_join addresses this (which
> rte_control_thread_create uses) and could remove ret parameter and to
> avoid touching the new function implementation in the future it can not
> use ret now.
> 
> i'll make this change in v3.
> 
> for the original function i will leave the code as is to minimize the
> diff. when rte_ctrl_thread_create is removed i'll make a note to
> eliminate the ret parameter as well.
> 

I would focus on minimizing the complexity of the end result, rather 
than the size of the patch.

>>
>>>   	/* Control thread status.
>>> @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
>>>   	enum __rte_ctrl_thread_status ctrl_thread_status;
>>>   };
>>> -static void *ctrl_thread_init(void *arg)
>>> +static int ctrl_thread_init(void *arg)
>>>   {
>>>   	struct internal_config *internal_conf =
>>>   		eal_get_internal_configuration();
>>>   	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>>>   	struct rte_thread_ctrl_params *params = arg;
>>> -	void *(*start_routine)(void *) = params->start_routine;
>>> -	void *routine_arg = params->arg;
>>>   	__rte_thread_init(rte_lcore_id(), cpuset);
>>>   	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
>>> @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
>>>   	if (params->ret != 0) {
>>>   		__atomic_store_n(&params->ctrl_thread_status,
>>>   			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
>>> -		return NULL;
>>> +		return params->ret;
>>>   	}
>>>   	__atomic_store_n(&params->ctrl_thread_status,
>>>   		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
>>> -	return start_routine(routine_arg);
>>> +	return 0;
>>> +}
>>> +
>>> +static void *ctrl_thread_start(void *arg)
>>> +{
>>> +	struct rte_thread_ctrl_params *params = arg;
>>> +	void *(*start_routine)(void *) = params->u.start_routine;
>>> +
>>> +	if (ctrl_thread_init(arg) != 0)
>>> +		return NULL;
>>> +
>>> +	return start_routine(params->arg);
>>> +}
>>> +
>>> +static uint32_t control_thread_start(void *arg)
>>> +{
>>> +	struct rte_thread_ctrl_params *params = arg;
>>> +	rte_thread_func start_routine = params->u.thread_func;
>>> +
>>> +	if (ctrl_thread_init(arg) != 0)
>>> +		return params->ret;
>>> +
>>> +	return start_routine(params->arg);
>>>   }
>>>   int
>>> @@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
>>>   	if (!params)
>>>   		return -ENOMEM;
>>> -	params->start_routine = start_routine;
>>> +	params->u.start_routine = start_routine;
>>>   	params->arg = arg;
>>>   	params->ret = 0;
>>>   	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>>> -	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>>> +	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
>>>   	if (ret != 0) {
>>>   		free(params);
>>>   		return -ret;
>>> @@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
>>>   }
>>>   int
>>> +rte_control_thread_create(rte_thread_t *thread, const char *name,
>>> +	const rte_thread_attr_t *attr,
>>> +	rte_thread_func start_routine, void *arg)
>>> +{
>>> +	struct rte_thread_ctrl_params *params;
>>> +	enum __rte_ctrl_thread_status ctrl_thread_status;
>>> +	int ret;
>>> +
>>> +	params = malloc(sizeof(*params));
>>> +	if (!params)
>>
>> params == NULL
> 
> copied from original code, i'll fix the style in the new function to
> comply with the dpdk coding standard.
> 
> i'll fix in v3.
> 
>>
>>> +		return -ENOMEM;
>>> +
>>> +	params->u.thread_func = start_routine;
>>> +	params->arg = arg;
>>> +	params->ret = 0;
>>> +	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>>> +
>>> +	ret = rte_thread_create(thread, attr, control_thread_start, params);
>>> +	if (ret != 0) {
>>> +		free(params);
>>> +		return -ret;
>>> +	}
>>> +
>>> +	if (name != NULL) {
>>> +		ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
>>> +		if (ret < 0)
>>> +			RTE_LOG(DEBUG, EAL,
>>> +				"Cannot set name for ctrl thread\n");
>>> +	}
>>> +
>>> +	/* Wait for the control thread to initialize successfully */
>>> +	while ((ctrl_thread_status =
>>> +			__atomic_load_n(&params->ctrl_thread_status,
>>> +			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
>>> +		/* Yield the CPU. Using sched_yield call requires maintaining
>>> +		 * another implementation for Windows as sched_yield is not
>>> +		 * supported on Windows.
>>> +		 */
>>
>> sched_yield() also doesn't necessarily yield the CPU.
> 
> copied from original code and understood, but are you requesting a
> change here or just a comment correction? can you offer wording you
> would find suitable?
> 

I would just remove the comment.

"Yield the CPU. sched_yield() may seem like a natural choice here, but 
does not guarantee that a context switch will actually occur, and also 
does not exist on Windows."


>>
>>> +		rte_delay_us_sleep(1);
>>> +	}
>>> +
>>> +	/* Check if the control thread encountered an error */
>>> +	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
>>> +		/* ctrl thread is exiting */
>>> +		rte_thread_join(*thread, NULL);
>>> +	}
>>> +
>>> +	ret = params->ret;
>>> +	free(params);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +int
>>>   rte_thread_register(void)
>>>   {
>>>   	unsigned int lcore_id;
>>> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
>>> index b9edf70..ae7afbe 100644
>>> --- a/lib/eal/include/rte_thread.h
>>> +++ b/lib/eal/include/rte_thread.h
>>> @@ -95,6 +95,35 @@ int rte_thread_create(rte_thread_t *thread_id,
>>>   		rte_thread_func thread_func, void *arg);
>>>   /**
>>> + * Create a control thread.
>>> + *
>>> + * Creates a control thread with the given name and attributes. The
>>> + * affinity of the new thread is based on the CPU affinity retrieved
>>> + * at the time rte_eal_init() was called, the dataplane and service
>>> + * lcores are then excluded. If setting the name of the thread fails,
>>
>> "the EAL threads are then excluded"
> 
> i'll modify in v3.
> 
>>
>>> + * the error is ignored and a debug message is logged.
>>> + *
>>> + * @param thread
>>> + *   Filled with the thread id of the new created thread.
>>> + * @param name
>>> + *   The name of the control thread (max 16 characters including '\0').
>>
>> Is there a constant for this limit?
> 
> i have a series introducing rte_lcore_{set,get}_name api that introduces
> a constant (probably i'll post it today). as of this series there is no
> constant.
> 
>>
>>> + * @param thread_attr
>>> + *   Attributes for the new thread.
>>> + * @param thread_func
>>> + *   Function to be executed by the new thread.
>>> + * @param arg
>>> + *   Argument passed to start_routine.
>>> + * @return
>>> + *   On success, returns 0; on error, it returns a negative value
>>> + *   corresponding to the error number.
>>
>> List the possible error codes.
> 
> i would like to see the range of codes be part of the api & abi i've
> received resistance to the idea. for now i'll nack this comment which
> leaves it consistent with other apis documentation.
>>
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_control_thread_create(rte_thread_t *thread, const char *name,
>>> +		const rte_thread_attr_t *thread_attr,
>>> +		rte_thread_func thread_func, void *arg);
>>> +
>>> +/**
>>>    * @warning
>>>    * @b EXPERIMENTAL: this API may change without prior notice.
>>>    *
>>> diff --git a/lib/eal/version.map b/lib/eal/version.map
>>> index 7ad12a7..8f9eeb9 100644
>>> --- a/lib/eal/version.map
>>> +++ b/lib/eal/version.map
>>> @@ -440,6 +440,9 @@ EXPERIMENTAL {
>>>   	rte_thread_detach;
>>>   	rte_thread_equal;
>>>   	rte_thread_join;
>>> +
>>> +	# added in 23.03
>>> +	rte_control_thread_create;
>>>   };
>>>   INTERNAL {

Re: [PATCH v2 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

On Thu, Dec 08, 2022 at 10:59:13PM +0100, Mattias Rönnblom wrote:
> On 2022-12-07 17:38, Tyler Retzlaff wrote:
> >
> >>
> >>>+	} u;
> >>>  	void *arg;
> >>>  	int ret;
> >>
> >>Why is 'ret' needed? (This question is unrelated to your patch.)
> >
> >i'm not the original author so difficult to answer authoritatively. but
> >if i have to speculate i'd say it might be to work around the windows
> >pthread_join stub being implemented as a noop. specifically it didn't
> >communicate the return value from the start_routine.
> >
> >the recently added rte_thread_join addresses this (which
> >rte_control_thread_create uses) and could remove ret parameter and to
> >avoid touching the new function implementation in the future it can not
> >use ret now.
> >
> >i'll make this change in v3.
> >
> >for the original function i will leave the code as is to minimize the
> >diff. when rte_ctrl_thread_create is removed i'll make a note to
> >eliminate the ret parameter as well.
> >
> 
> I would focus on minimizing the complexity of the end result, rather
> than the size of the patch.

agreed, that's why i decided to leave it as is. more change than is
needed right now and simpler to make after the old api is removed.

> >>>+	/* Wait for the control thread to initialize successfully */
> >>>+	while ((ctrl_thread_status =
> >>>+			__atomic_load_n(&params->ctrl_thread_status,
> >>>+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> >>>+		/* Yield the CPU. Using sched_yield call requires maintaining
> >>>+		 * another implementation for Windows as sched_yield is not
> >>>+		 * supported on Windows.
> >>>+		 */
> >>
> >>sched_yield() also doesn't necessarily yield the CPU.
> >
> >copied from original code and understood, but are you requesting a
> >change here or just a comment correction? can you offer wording you
> >would find suitable?
> >
> 
> I would just remove the comment.

sounds good, i'll remove it in the next rev.

thanks!

[PATCH v4 0/3] eal: rte_ctrl_thread_create API replacement

[Tyler Retzlaff]

Remove rte_ctrl_thread_create API which exposes pthread_t and provide
new rte_control_thread_create API based on EALs rte_thread_t.

To limit compatibility regression risk and ease of removal of the
existing rte_ctrl_thread_create in the future duplicate most of the
existing implementation. The duplication will be removed when
rte_ctrl_thread_create is finally duplicated/removed.

The unit test for rte_ctrl_thread_create has been duplicated to exercise
both the old and the new API during transition, as with the API the
duplicate test will be removed when the rte_ctrl_thread_create API is
removed.

v4:
  * fix missing whitespace in deprecation notice text
  * remove comment in rte_control_thread_create implementation
    referring to sched_yield as requested by community feedback
  * add missing parameter name to function pointer declaration

v3:
  * use {ctrl,control}_start_routine for start_routine field names
  * fix conditional evaluation style p == NULL instead of !p
  * tweak documentation comment for rte_control_thread_create
    - "the EAL threads are then excluded"
    - note RTE_MAX_THREAD_NAME_LEN preprocessor definition
      is the name size (including terminating NUL) limit
  * add missing cast to uintptr_t

v2:
  * correct style error void * (*foo) -> void *(*foo)
  * place retval on lhs of comparison 0 != foo() -> foo() != 0
  * add missing commit description on patch 3/3
  * add cast uintptr_t to pthread_t where appropriate
  * fix doxygen @param names to match parameter names

Tyler Retzlaff (3):
  eal: add rte control thread create API
  test: add rte control thread create API test
  eal: deprecate pthread control thread create API

 app/test/test_lcores.c               | 42 +++++++++++++++++
 doc/guides/rel_notes/deprecation.rst |  4 ++
 lib/eal/common/eal_common_thread.c   | 89 ++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h         | 30 ++++++++++++
 lib/eal/version.map                  |  3 ++
 5 files changed, 160 insertions(+), 8 deletions(-)

-- 
1.8.3.1

Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v4 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

Add rte_control_thread_create API as a replacement for
rte_ctrl_thread_create to allow deprecation of the use of platform
specific types in DPDK public API.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/common/eal_common_thread.c | 89 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 30 +++++++++++++
 lib/eal/version.map                |  3 ++
 3 files changed, 114 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index c5d8b43..872ad20 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void *(*ctrl_start_routine)(void *arg);
+		rte_thread_func control_start_routine;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
@@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.ctrl_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.control_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.ctrl_start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -322,6 +345,56 @@ static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (params == NULL)
+		return -ENOMEM;
+
+	params->u.control_start_routine = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL) {
+		ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
+		if (ret < 0)
+			RTE_LOG(DEBUG, EAL,
+				"Cannot set name for ctrl thread\n");
+	}
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index b9edf70..97c27a9 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -95,6 +95,36 @@ int rte_thread_create(rte_thread_t *thread_id,
 		rte_thread_func thread_func, void *arg);
 
 /**
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the EAL threads are then
+ * excluded. If setting the name of the thread fails, the error is
+ * ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread
+ *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
+ * @param thread_attr
+ *   Attributes for the new thread.
+ * @param thread_func
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7..8f9eeb9 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,9 @@ EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+
+	# added in 23.03
+	rte_control_thread_create;
 };
 
 INTERNAL {
-- 
1.8.3.1

[PATCH v4 2/3] test: add rte control thread create API test

[Tyler Retzlaff]

Duplicate the rte_ctrl_thread_create test adapted to use
rte_control_thread create to keep both apis under test until
rte_ctrl_thread_create is removed.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 app/test/test_lcores.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index a6bb412..375ecdb 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -8,6 +8,7 @@
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_lcore.h>
+#include <rte_thread.h>
 
 #include "test.h"
 
@@ -352,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
 	return NULL;
 }
 
+static uint32_t control_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = Thread_DONE;
+
+	return 0;
+}
+
 static int
 test_ctrl_thread(void)
 {
@@ -379,6 +392,32 @@ static void *ctrl_thread_loop(void *arg)
 }
 
 static int
+test_control_thread(void)
+{
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = Thread_INIT;
+	if (rte_control_thread_create((rte_thread_t *)&t->id, "test_control_threads",
+					NULL, control_thread_loop, t) != 0)
+		return -1;
+
+	/* Wait till the control thread exits.
+	 * This also acts as the barrier such that the memory operations
+	 * in control thread are visible to this thread.
+	 */
+	rte_thread_join((rte_thread_t){(uintptr_t)t->id}, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != Thread_DONE)
+		return -1;
+
+	return 0;
+}
+
+static int
 test_lcores(void)
 {
 	unsigned int eal_threads_count = 0;
@@ -411,6 +450,9 @@ static void *ctrl_thread_loop(void *arg)
 	if (test_ctrl_thread() < 0)
 		return TEST_FAILED;
 
+	if (test_control_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
-- 
1.8.3.1

[PATCH v4 3/3] eal: deprecate pthread control thread create API

[Tyler Retzlaff]

Give notice of rte_ctrl_thread_create deprecation.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b9b02dc..ce3b4b7 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -119,3 +119,7 @@ Deprecation Notices
   Its removal has been postponed to let potential users report interest
   in maintaining it.
   In the absence of such interest, this library will be removed in DPDK 23.11.
+
+* eal: The function ``rte_ctrl_thread_create`` will be removed and
+  replaced by the new ``rte_control_thread_create`` api, continuing the
+  effort to decouple eal from platform-specific thread implementations.
-- 
1.8.3.1

Re: [PATCH v2 1/3] eal: add rte control thread create API

[Stephen Hemminger]

On Tue,  6 Dec 2022 09:28:24 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> +	/* Wait for the control thread to initialize successfully */
> +	while ((ctrl_thread_status =
> +			__atomic_load_n(&params->ctrl_thread_status,
> +			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> +		/* Yield the CPU. Using sched_yield call requires maintaining
> +		 * another implementation for Windows as sched_yield is not
> +		 * supported on Windows.
> +		 */
> +		rte_delay_us_sleep(1);
> +	}

Would it be worth introducing and using rte_thread_yield().
Rather than waiting 1us which seems like a long time for this.

Re: [PATCH v2 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

On Thu, Dec 08, 2022 at 05:09:32PM -0800, Stephen Hemminger wrote:
> On Tue,  6 Dec 2022 09:28:24 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > +	/* Wait for the control thread to initialize successfully */
> > +	while ((ctrl_thread_status =
> > +			__atomic_load_n(&params->ctrl_thread_status,
> > +			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> > +		/* Yield the CPU. Using sched_yield call requires maintaining
> > +		 * another implementation for Windows as sched_yield is not
> > +		 * supported on Windows.
> > +		 */
> > +		rte_delay_us_sleep(1);
> > +	}
> 
> Would it be worth introducing and using rte_thread_yield().
> Rather than waiting 1us which seems like a long time for this.

i'd have to figure out if it can be provided for all platforms, if so
then it seems reasonable. though i would not introduce it in this patch
series since it isn't the central purpose of this change. more than
happy to do it in a new series.

of course if someone else wants to put up a patch to add it i'm more
than happy to review that too. (i have a lot of patches in the pipe
already i'd like to avoid getting too distracted).

i'll put it in my backlog to take a look.

thanks

Re: [PATCH v4 0/3] eal: rte_ctrl_thread_create API replacement

[Mattias Rönnblom]

On 2022-12-09 00:19, Tyler Retzlaff wrote:
> Remove rte_ctrl_thread_create API which exposes pthread_t and provide
> new rte_control_thread_create API based on EALs rte_thread_t.
> 

<snip>

For the series,
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

[PATCH v5 0/3] eal: deprecate last use of pthread_t in public API

[Tyler Retzlaff]

Announce deprecation of rte_ctrl_thread_create API which is the final
remaining stable API exposing pthread_t.

Provide an equivalent replacement API rte_control_thread_create that
uses the new rte_thread_t type.

Provide a unit test for the new rte_control_thread_create API.

Add missing doxygen index for thread / rte_thread.h.

Notice!

To limit compatibility regression risk and ease removal of the
existing rte_ctrl_thread_create in the future duplicate most of the
existing implementation rather than try to have it accommodate both
public API contracts.

The duplication, the union introduced to support it along with remaining
internal pthread_xxx calls will be removed when rte_ctrl_thread_create is
finally removed.

The old unit test for rte_ctrl_thread_create is kept in place to guarantee
correct behavior while deprecated and will be removed when
rte_ctrl_thread_create is finally removed.

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

v5:
  * rebase series now that rte_thread_set_name has been merged,
    remove now unnecessary casts.
  * combine patch adding rte_control_thread_create and patch
    adding unit test into patch 1.
  * reword deprecation notice to indicate which release we
    intend to mark rte_ctrl_thread_create deprecated and which
    release we intend to remove the same.
  * adjust deprecation notice commit subject based on previous
    deprecation notice series feedback.
  * add (mostly unrelated) patch to series to add thread / rte_thread.h
    to doxygen index (an oversight missed from previous series).

v4:
  * fix missing whitespace in deprecation notice text
  * remove comment in rte_control_thread_create implementation
    referring to sched_yield as requested by community feedback
  * add missing parameter name to function pointer declaration

v3:
  * use {ctrl,control}_start_routine for start_routine field names
  * fix conditional evaluation style p == NULL instead of !p
  * tweak documentation comment for rte_control_thread_create
    - "the EAL threads are then excluded"
    - note RTE_MAX_THREAD_NAME_LEN preprocessor definition
      is the name size (including terminating NUL) limit
  * add missing cast to uintptr_t

v2:
  * correct style error void * (*foo) -> void *(*foo)
  * place retval on lhs of comparison 0 != foo() -> foo() != 0
  * add missing commit description on patch 3/3
  * add cast uintptr_t to pthread_t where appropriate
  * fix doxygen @param names to match parameter names

Tyler Retzlaff (3):
  eal: add rte control thread create API
  doc: add missing index entry for thread
  doc: announce deprecation of thread ctrl create function

 app/test/test_lcores.c               | 41 +++++++++++++++++
 doc/api/doxy-api-index.md            |  1 +
 doc/guides/rel_notes/deprecation.rst |  5 +++
 lib/eal/common/eal_common_thread.c   | 85 ++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h         | 33 ++++++++++++++
 lib/eal/version.map                  |  1 +
 6 files changed, 158 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH v5 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

Add rte_control_thread_create API as a replacement for
rte_ctrl_thread_create to allow deprecation of the use of platform
specific types in DPDK public API.

Duplicate the rte_ctrl_thread_create test adapted to use
rte_control_thread create to keep both APIs under test until
rte_ctrl_thread_create is removed.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 app/test/test_lcores.c             | 41 ++++++++++++++++++
 lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 33 +++++++++++++++
 lib/eal/version.map                |  1 +
 4 files changed, 152 insertions(+), 8 deletions(-)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index 5b43aa5..9766f78 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -353,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
 	return NULL;
 }
 
+static uint32_t control_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = Thread_DONE;
+
+	return 0;
+}
+
 static int
 test_ctrl_thread(void)
 {
@@ -380,6 +392,32 @@ static void *ctrl_thread_loop(void *arg)
 }
 
 static int
+test_control_thread(void)
+{
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = Thread_INIT;
+	if (rte_control_thread_create(&t->id, "test_control_threads",
+					NULL, control_thread_loop, t) != 0)
+		return -1;
+
+	/* Wait till the control thread exits.
+	 * This also acts as the barrier such that the memory operations
+	 * in control thread are visible to this thread.
+	 */
+	rte_thread_join(t->id, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != Thread_DONE)
+		return -1;
+
+	return 0;
+}
+
+static int
 test_lcores(void)
 {
 	unsigned int eal_threads_count = 0;
@@ -409,6 +447,9 @@ static void *ctrl_thread_loop(void *arg)
 	if (test_ctrl_thread() < 0)
 		return TEST_FAILED;
 
+	if (test_control_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 3181515..7ea216e 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void *(*ctrl_start_routine)(void *arg);
+		rte_thread_func control_start_routine;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -241,27 +244,47 @@ struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.ctrl_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.control_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.ctrl_start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (params == NULL)
+		return -ENOMEM;
+
+	params->u.control_start_routine = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL)
+		rte_thread_set_name(*thread, name);
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index d247930..792d8b0 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the EAL threads are then
+ * excluded. If setting the name of the thread fails, the error is
+ * ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread
+ *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
+ * @param thread_attr
+ *   Attributes for the new thread.
+ * @param thread_func
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
  * Waits for the thread identified by 'thread_id' to terminate
  *
  * @param thread_id
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 6523102..a9d0a03 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -441,6 +441,7 @@ EXPERIMENTAL {
 	rte_thread_join;
 
 	# added in 23.03
+	rte_control_thread_create;
 	rte_thread_set_name;
 };
 
-- 
1.8.3.1

[PATCH v5 2/3] doc: add missing index entry for thread

[Tyler Retzlaff]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

Add missing thread index referencing rte_thread.h under the basic
topic.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/api/doxy-api-index.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
index de488c7..e8ed51a 100644
--- a/doc/api/doxy-api-index.md
+++ b/doc/api/doxy-api-index.md
@@ -211,6 +211,7 @@ The public API headers are grouped by topics:
   [config file](@ref rte_cfgfile.h),
   [key/value args](@ref rte_kvargs.h),
   [string](@ref rte_string_fns.h)
+  [thread](@ref rte_thread.h)
 
 - **debug**:
   [jobstats](@ref rte_jobstats.h),
-- 
1.8.3.1

[PATCH v5 3/3] doc: announce deprecation of thread ctrl create function

[Tyler Retzlaff]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

Notify deprecation of rte_ctrl_thread_create API, it will be removed as
it exposes platform-specific thread details.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index eea99b4..c6e7d04 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -23,6 +23,11 @@ Deprecation Notices
   rte_thread_set_name being marked as stable, and planned to be removed
   by the 23.11 release.
 
+* eal: The function ``rte_ctrl_thread_create`` is planned to be deprecated
+  starting with the 23.07 release, subject to the replacement API
+  rte_control_thread_create being marked as stable, and planned to be removed
+  by the 23.11 release.
+
 * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
   not allow for writing optimized code for all the CPU architectures supported
   in DPDK. DPDK has adopted the atomic operations from
-- 
1.8.3.1

[PATCH v5 0/3] eal: deprecate last use of pthread_t in public API

[Tyler Retzlaff]

Announce deprecation of rte_ctrl_thread_create API which is the final
remaining stable API exposing pthread_t.

Provide an equivalent replacement API rte_control_thread_create that
uses the new rte_thread_t type.

Provide a unit test for the new rte_control_thread_create API.

Add missing doxygen index for thread / rte_thread.h.

Notice!

To limit compatibility regression risk and ease removal of the
existing rte_ctrl_thread_create in the future duplicate most of the
existing implementation rather than try to have it accommodate both
public API contracts.

The duplication, the union introduced to support it along with remaining
internal pthread_xxx calls will be removed when rte_ctrl_thread_create is
finally removed.

The old unit test for rte_ctrl_thread_create is kept in place to guarantee
correct behavior while deprecated and will be removed when
rte_ctrl_thread_create is finally removed.

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

v5:
  * rebase series now that rte_thread_set_name has been merged,
    remove now unnecessary casts.
  * combine patch adding rte_control_thread_create and patch
    adding unit test into patch 1.
  * reword deprecation notice to indicate which release we
    intend to mark rte_ctrl_thread_create deprecated and which
    release we intend to remove the same.
  * adjust deprecation notice commit subject based on previous
    deprecation notice series feedback.
  * add (mostly unrelated) patch to series to add thread / rte_thread.h
    to doxygen index (an oversight missed from previous series).

v4:
  * fix missing whitespace in deprecation notice text
  * remove comment in rte_control_thread_create implementation
    referring to sched_yield as requested by community feedback
  * add missing parameter name to function pointer declaration

v3:
  * use {ctrl,control}_start_routine for start_routine field names
  * fix conditional evaluation style p == NULL instead of !p
  * tweak documentation comment for rte_control_thread_create
    - "the EAL threads are then excluded"
    - note RTE_MAX_THREAD_NAME_LEN preprocessor definition
      is the name size (including terminating NUL) limit
  * add missing cast to uintptr_t

v2:
  * correct style error void * (*foo) -> void *(*foo)
  * place retval on lhs of comparison 0 != foo() -> foo() != 0
  * add missing commit description on patch 3/3
  * add cast uintptr_t to pthread_t where appropriate
  * fix doxygen @param names to match parameter names

Tyler Retzlaff (3):
  eal: add rte control thread create API
  doc: add missing index entry for thread
  doc: announce deprecation of thread ctrl create function

 app/test/test_lcores.c               | 41 +++++++++++++++++
 doc/api/doxy-api-index.md            |  1 +
 doc/guides/rel_notes/deprecation.rst |  5 +++
 lib/eal/common/eal_common_thread.c   | 85 ++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h         | 33 ++++++++++++++
 lib/eal/version.map                  |  1 +
 6 files changed, 158 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH v5 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

Add rte_control_thread_create API as a replacement for
rte_ctrl_thread_create to allow deprecation of the use of platform
specific types in DPDK public API.

Duplicate the rte_ctrl_thread_create test adapted to use
rte_control_thread create to keep both APIs under test until
rte_ctrl_thread_create is removed.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 app/test/test_lcores.c             | 41 ++++++++++++++++++
 lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 33 +++++++++++++++
 lib/eal/version.map                |  1 +
 4 files changed, 152 insertions(+), 8 deletions(-)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index 5b43aa5..9766f78 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -353,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
 	return NULL;
 }
 
+static uint32_t control_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = Thread_DONE;
+
+	return 0;
+}
+
 static int
 test_ctrl_thread(void)
 {
@@ -380,6 +392,32 @@ static void *ctrl_thread_loop(void *arg)
 }
 
 static int
+test_control_thread(void)
+{
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = Thread_INIT;
+	if (rte_control_thread_create(&t->id, "test_control_threads",
+					NULL, control_thread_loop, t) != 0)
+		return -1;
+
+	/* Wait till the control thread exits.
+	 * This also acts as the barrier such that the memory operations
+	 * in control thread are visible to this thread.
+	 */
+	rte_thread_join(t->id, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != Thread_DONE)
+		return -1;
+
+	return 0;
+}
+
+static int
 test_lcores(void)
 {
 	unsigned int eal_threads_count = 0;
@@ -409,6 +447,9 @@ static void *ctrl_thread_loop(void *arg)
 	if (test_ctrl_thread() < 0)
 		return TEST_FAILED;
 
+	if (test_control_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 3181515..7ea216e 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void *(*ctrl_start_routine)(void *arg);
+		rte_thread_func control_start_routine;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -241,27 +244,47 @@ struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.ctrl_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.control_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.ctrl_start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (params == NULL)
+		return -ENOMEM;
+
+	params->u.control_start_routine = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL)
+		rte_thread_set_name(*thread, name);
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index d247930..792d8b0 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the EAL threads are then
+ * excluded. If setting the name of the thread fails, the error is
+ * ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread
+ *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
+ * @param thread_attr
+ *   Attributes for the new thread.
+ * @param thread_func
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
  * Waits for the thread identified by 'thread_id' to terminate
  *
  * @param thread_id
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 6523102..a9d0a03 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -441,6 +441,7 @@ EXPERIMENTAL {
 	rte_thread_join;
 
 	# added in 23.03
+	rte_control_thread_create;
 	rte_thread_set_name;
 };
 
-- 
1.8.3.1

[PATCH v5 2/3] doc: add missing index entry for thread

[Tyler Retzlaff]

Add missing thread index referencing rte_thread.h under the basic
topic.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/api/doxy-api-index.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
index de488c7..e8ed51a 100644
--- a/doc/api/doxy-api-index.md
+++ b/doc/api/doxy-api-index.md
@@ -211,6 +211,7 @@ The public API headers are grouped by topics:
   [config file](@ref rte_cfgfile.h),
   [key/value args](@ref rte_kvargs.h),
   [string](@ref rte_string_fns.h)
+  [thread](@ref rte_thread.h)
 
 - **debug**:
   [jobstats](@ref rte_jobstats.h),
-- 
1.8.3.1

[PATCH v5 3/3] doc: announce deprecation of thread ctrl create function

[Tyler Retzlaff]

Notify deprecation of rte_ctrl_thread_create API, it will be removed as
it exposes platform-specific thread details.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index eea99b4..c6e7d04 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -23,6 +23,11 @@ Deprecation Notices
   rte_thread_set_name being marked as stable, and planned to be removed
   by the 23.11 release.
 
+* eal: The function ``rte_ctrl_thread_create`` is planned to be deprecated
+  starting with the 23.07 release, subject to the replacement API
+  rte_control_thread_create being marked as stable, and planned to be removed
+  by the 23.11 release.
+
 * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
   not allow for writing optimized code for all the CPU architectures supported
   in DPDK. DPDK has adopted the atomic operations from
-- 
1.8.3.1

Re: [PATCH v5 0/3] eal: deprecate last use of pthread_t in public API

[Tyler Retzlaff]

top-post

this is a re-submission of v5 of the series due to a mistake i made
in the first submission that caused it not to be correctly submitted
to patchwork.

apologies for the noise.
    
On Tue, Jan 31, 2023 at 12:30:14PM -0800, Tyler Retzlaff wrote:
> Announce deprecation of rte_ctrl_thread_create API which is the final
> remaining stable API exposing pthread_t.
> 
> Provide an equivalent replacement API rte_control_thread_create that
> uses the new rte_thread_t type.
> 
> Provide a unit test for the new rte_control_thread_create API.
> 
> Add missing doxygen index for thread / rte_thread.h.
> 
> Notice!
> 
> To limit compatibility regression risk and ease removal of the
> existing rte_ctrl_thread_create in the future duplicate most of the
> existing implementation rather than try to have it accommodate both
> public API contracts.
> 
> The duplication, the union introduced to support it along with remaining
> internal pthread_xxx calls will be removed when rte_ctrl_thread_create is
> finally removed.
> 
> The old unit test for rte_ctrl_thread_create is kept in place to guarantee
> correct behavior while deprecated and will be removed when
> rte_ctrl_thread_create is finally removed.
> 
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> 
> v5:
>   * rebase series now that rte_thread_set_name has been merged,
>     remove now unnecessary casts.
>   * combine patch adding rte_control_thread_create and patch
>     adding unit test into patch 1.
>   * reword deprecation notice to indicate which release we
>     intend to mark rte_ctrl_thread_create deprecated and which
>     release we intend to remove the same.
>   * adjust deprecation notice commit subject based on previous
>     deprecation notice series feedback.
>   * add (mostly unrelated) patch to series to add thread / rte_thread.h
>     to doxygen index (an oversight missed from previous series).
> 
> v4:
>   * fix missing whitespace in deprecation notice text
>   * remove comment in rte_control_thread_create implementation
>     referring to sched_yield as requested by community feedback
>   * add missing parameter name to function pointer declaration
> 
> v3:
>   * use {ctrl,control}_start_routine for start_routine field names
>   * fix conditional evaluation style p == NULL instead of !p
>   * tweak documentation comment for rte_control_thread_create
>     - "the EAL threads are then excluded"
>     - note RTE_MAX_THREAD_NAME_LEN preprocessor definition
>       is the name size (including terminating NUL) limit
>   * add missing cast to uintptr_t
> 
> v2:
>   * correct style error void * (*foo) -> void *(*foo)
>   * place retval on lhs of comparison 0 != foo() -> foo() != 0
>   * add missing commit description on patch 3/3
>   * add cast uintptr_t to pthread_t where appropriate
>   * fix doxygen @param names to match parameter names
> 
> Tyler Retzlaff (3):
>   eal: add rte control thread create API
>   doc: add missing index entry for thread
>   doc: announce deprecation of thread ctrl create function
> 
>  app/test/test_lcores.c               | 41 +++++++++++++++++
>  doc/api/doxy-api-index.md            |  1 +
>  doc/guides/rel_notes/deprecation.rst |  5 +++
>  lib/eal/common/eal_common_thread.c   | 85 ++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h         | 33 ++++++++++++++
>  lib/eal/version.map                  |  1 +
>  6 files changed, 158 insertions(+), 8 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [PATCH v5 1/3] eal: add rte control thread create API

[David Marchand]

On Tue, Jan 31, 2023 at 9:30 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Add rte_control_thread_create API as a replacement for
> rte_ctrl_thread_create to allow deprecation of the use of platform
> specific types in DPDK public API.
>
> Duplicate the rte_ctrl_thread_create test adapted to use
> rte_control_thread create to keep both APIs under test until
> rte_ctrl_thread_create is removed.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
>  app/test/test_lcores.c             | 41 ++++++++++++++++++
>  lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h       | 33 +++++++++++++++
>  lib/eal/version.map                |  1 +
>  4 files changed, 152 insertions(+), 8 deletions(-)
>
> diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
> index 5b43aa5..9766f78 100644
> --- a/app/test/test_lcores.c
> +++ b/app/test/test_lcores.c
> @@ -353,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
>         return NULL;
>  }
>
> +static uint32_t control_thread_loop(void *arg)
> +{
> +       struct thread_context *t = arg;
> +
> +       printf("Control thread running successfully\n");
> +
> +       /* Set the thread state to DONE */
> +       t->state = Thread_DONE;
> +
> +       return 0;
> +}
> +
>  static int
>  test_ctrl_thread(void)
>  {
> @@ -380,6 +392,32 @@ static void *ctrl_thread_loop(void *arg)
>  }
>
>  static int
> +test_control_thread(void)
> +{
> +       struct thread_context ctrl_thread_context;
> +       struct thread_context *t;
> +
> +       /* Create one control thread */
> +       t = &ctrl_thread_context;
> +       t->state = Thread_INIT;
> +       if (rte_control_thread_create(&t->id, "test_control_threads",
> +                                       NULL, control_thread_loop, t) != 0)
> +               return -1;
> +
> +       /* Wait till the control thread exits.
> +        * This also acts as the barrier such that the memory operations
> +        * in control thread are visible to this thread.
> +        */
> +       rte_thread_join(t->id, NULL);
> +
> +       /* Check if the control thread set the correct state */
> +       if (t->state != Thread_DONE)
> +               return -1;
> +
> +       return 0;
> +}
> +
> +static int
>  test_lcores(void)
>  {
>         unsigned int eal_threads_count = 0;
> @@ -409,6 +447,9 @@ static void *ctrl_thread_loop(void *arg)
>         if (test_ctrl_thread() < 0)
>                 return TEST_FAILED;
>
> +       if (test_control_thread() < 0)
> +               return TEST_FAILED;
> +
>         return TEST_SUCCESS;
>  }
>

Afair, the "legacy" API test being in test_lcores.c is mainly a side
effect of the API being defined in rte_lcore.h.

The new API is genuinely located in rte_thread.h and there is no
consideration over lcores: control thread are just "specialised"
rte_thread objects.
I'd rather see this test in app/test/test_threads.c with other
rte_thread API tests.

I think something like below would be enough, wdyt?
If you are fine with it and there is no other comment on this patch, I
plan to do this change before applying.


diff --git a/app/test/test_threads.c b/app/test/test_threads.c
index e0f18e4329..cc0bb69190 100644
--- a/app/test/test_threads.c
+++ b/app/test/test_threads.c
@@ -232,6 +232,31 @@ test_thread_attributes_priority(void)
        return 0;
 }

+static int
+test_control_thread_create_join(void)
+{
+       rte_thread_t thread_id;
+       rte_thread_t thread_main_id;
+
+       thread_id_ready = 0;
+       RTE_TEST_ASSERT(rte_control_thread_create(&thread_id,
"test_control_threads", NULL,
+               thread_main, &thread_main_id) == 0,
+               "Failed to create thread.");
+
+       while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) == 0)
+               ;
+
+       RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) != 0,
+               "Unexpected thread id.");
+
+       __atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE);
+
+       RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) == 0,
+               "Failed to join thread.");
+
+       return 0;
+}
+
 static struct unit_test_suite threads_test_suite = {
        .suite_name = "threads autotest",
        .setup = NULL,
@@ -243,6 +268,7 @@ static struct unit_test_suite threads_test_suite = {
                TEST_CASE(test_thread_priority),
                TEST_CASE(test_thread_attributes_affinity),
                TEST_CASE(test_thread_attributes_priority),
+               TEST_CASE(test_control_thread_create_join),
                TEST_CASES_END()
        }
 };


-- 
David Marchand

Re: [PATCH v5 2/3] doc: add missing index entry for thread

[David Marchand]

On Tue, Jan 31, 2023 at 9:30 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Add missing thread index referencing rte_thread.h under the basic
> topic.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  doc/api/doxy-api-index.md | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index de488c7..e8ed51a 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -211,6 +211,7 @@ The public API headers are grouped by topics:
>    [config file](@ref rte_cfgfile.h),
>    [key/value args](@ref rte_kvargs.h),
>    [string](@ref rte_string_fns.h)
> +  [thread](@ref rte_thread.h)

A , is missing on the line before.
This can be seen in the rendered html:
"basic: bitops, approx fraction, random, config file, key/value args,
string thread"

I'll fix when applying.

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH v5 3/3] doc: announce deprecation of thread ctrl create function

[David Marchand]

On Tue, Jan 31, 2023 at 9:30 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Notify deprecation of rte_ctrl_thread_create API, it will be removed as
> it exposes platform-specific thread details.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

We could merge together the two notices about thread API, but the
proposed form is ok for me.

Acked-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH v5 2/3] doc: add missing index entry for thread

[Tyler Retzlaff]

On Tue, Feb 07, 2023 at 04:39:06PM +0100, David Marchand wrote:
> On Tue, Jan 31, 2023 at 9:30 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Add missing thread index referencing rte_thread.h under the basic
> > topic.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  doc/api/doxy-api-index.md | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> > index de488c7..e8ed51a 100644
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -211,6 +211,7 @@ The public API headers are grouped by topics:
> >    [config file](@ref rte_cfgfile.h),
> >    [key/value args](@ref rte_kvargs.h),
> >    [string](@ref rte_string_fns.h)
> > +  [thread](@ref rte_thread.h)
> 
> A , is missing on the line before.
> This can be seen in the rendered html:
> "basic: bitops, approx fraction, random, config file, key/value args,
> string thread"

sorry, just me being a derp. i will fix it in the next version since
need a new rev for moving the test to test_threads.c

> 
> I'll fix when applying.
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> 
> -- 
> David Marchand

Re: [PATCH v5 1/3] eal: add rte control thread create API

[Tyler Retzlaff]

On Tue, Feb 07, 2023 at 02:12:18PM +0100, David Marchand wrote:
> On Tue, Jan 31, 2023 at 9:30 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Add rte_control_thread_create API as a replacement for
> > rte_ctrl_thread_create to allow deprecation of the use of platform
> > specific types in DPDK public API.
> >
> > Duplicate the rte_ctrl_thread_create test adapted to use
> > rte_control_thread create to keep both APIs under test until
> > rte_ctrl_thread_create is removed.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > ---
> >  app/test/test_lcores.c             | 41 ++++++++++++++++++
> >  lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 33 +++++++++++++++
> >  lib/eal/version.map                |  1 +
> >  4 files changed, 152 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
> > index 5b43aa5..9766f78 100644
> > --- a/app/test/test_lcores.c
> > +++ b/app/test/test_lcores.c
> > @@ -353,6 +353,18 @@ static void *ctrl_thread_loop(void *arg)
> >         return NULL;
> >  }
> >
> > +static uint32_t control_thread_loop(void *arg)
> > +{
> > +       struct thread_context *t = arg;
> > +
> > +       printf("Control thread running successfully\n");
> > +
> > +       /* Set the thread state to DONE */
> > +       t->state = Thread_DONE;
> > +
> > +       return 0;
> > +}
> > +
> >  static int
> >  test_ctrl_thread(void)
> >  {
> > @@ -380,6 +392,32 @@ static void *ctrl_thread_loop(void *arg)
> >  }
> >
> >  static int
> > +test_control_thread(void)
> > +{
> > +       struct thread_context ctrl_thread_context;
> > +       struct thread_context *t;
> > +
> > +       /* Create one control thread */
> > +       t = &ctrl_thread_context;
> > +       t->state = Thread_INIT;
> > +       if (rte_control_thread_create(&t->id, "test_control_threads",
> > +                                       NULL, control_thread_loop, t) != 0)
> > +               return -1;
> > +
> > +       /* Wait till the control thread exits.
> > +        * This also acts as the barrier such that the memory operations
> > +        * in control thread are visible to this thread.
> > +        */
> > +       rte_thread_join(t->id, NULL);
> > +
> > +       /* Check if the control thread set the correct state */
> > +       if (t->state != Thread_DONE)
> > +               return -1;
> > +
> > +       return 0;
> > +}
> > +
> > +static int
> >  test_lcores(void)
> >  {
> >         unsigned int eal_threads_count = 0;
> > @@ -409,6 +447,9 @@ static void *ctrl_thread_loop(void *arg)
> >         if (test_ctrl_thread() < 0)
> >                 return TEST_FAILED;
> >
> > +       if (test_control_thread() < 0)
> > +               return TEST_FAILED;
> > +
> >         return TEST_SUCCESS;
> >  }
> >
> 
> Afair, the "legacy" API test being in test_lcores.c is mainly a side
> effect of the API being defined in rte_lcore.h.
> 
> The new API is genuinely located in rte_thread.h and there is no
> consideration over lcores: control thread are just "specialised"
> rte_thread objects.
> I'd rather see this test in app/test/test_threads.c with other
> rte_thread API tests.

no problem, i wondered if i should retain the original location or not.
i'll move it to test_threads.c

i kind of wonder if the function should be named rte_thread_create_ctrl
or something now so that all the functions from rte_thread have a
consistent naming prefix?

let me know if this is desired. if it is i'll submit a new version
otherwise you can apply the series as is with your suggested test moved
to test_threads.c below.

> 
> I think something like below would be enough, wdyt?
> If you are fine with it and there is no other comment on this patch, I
> plan to do this change before applying.

no problem with this, sounds good.

> 
> 
> diff --git a/app/test/test_threads.c b/app/test/test_threads.c
> index e0f18e4329..cc0bb69190 100644
> --- a/app/test/test_threads.c
> +++ b/app/test/test_threads.c
> @@ -232,6 +232,31 @@ test_thread_attributes_priority(void)
>         return 0;
>  }
> 
> +static int
> +test_control_thread_create_join(void)
> +{
> +       rte_thread_t thread_id;
> +       rte_thread_t thread_main_id;
> +
> +       thread_id_ready = 0;
> +       RTE_TEST_ASSERT(rte_control_thread_create(&thread_id,
> "test_control_threads", NULL,
> +               thread_main, &thread_main_id) == 0,
> +               "Failed to create thread.");
> +
> +       while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) == 0)
> +               ;
> +
> +       RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) != 0,
> +               "Unexpected thread id.");
> +
> +       __atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE);
> +
> +       RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) == 0,
> +               "Failed to join thread.");
> +
> +       return 0;
> +}
> +
>  static struct unit_test_suite threads_test_suite = {
>         .suite_name = "threads autotest",
>         .setup = NULL,
> @@ -243,6 +268,7 @@ static struct unit_test_suite threads_test_suite = {
>                 TEST_CASE(test_thread_priority),
>                 TEST_CASE(test_thread_attributes_affinity),
>                 TEST_CASE(test_thread_attributes_priority),
> +               TEST_CASE(test_control_thread_create_join),
>                 TEST_CASES_END()
>         }
>  };
> 
> 
> -- 
> David Marchand

[PATCH v6 0/3] eal: deprecate last use of pthread_t in public API

[Tyler Retzlaff]

Announce deprecation of rte_ctrl_thread_create API which is the final
remaining stable API exposing pthread_t.

Provide an equivalent replacement API rte_thread_create_control that
uses the new rte_thread_t type.

Provide a unit test for the new rte_thread_create_control API
(test code provided by David Marchand.)

Add missing doxygen index for thread / rte_thread.h.

Notice!

To limit compatibility regression risk and ease removal of the
existing rte_ctrl_thread_create in the future duplicate most of the
existing implementation rather than try to have it accommodate both
public API contracts.

The duplication, the union introduced to support it along with remaining
internal pthread_xxx calls will be removed when rte_ctrl_thread_create is
finally removed.

The old unit test for rte_ctrl_thread_create is kept in place to guarantee
correct behavior while deprecated and will be removed when
rte_ctrl_thread_create is finally removed.

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

v6:
  * fix missing comma in doxygen index
  * combine deprecation notice for rte_ctrl_thread_create into notice
    of removal of rte_thread_setname.
  * remove test_lcore.c test and add test provided by David Marchand
    to test_threads.c.
  * rename the function to start with rte_thread to be consistent
    with the rest of functions in the rte_thread.h APIs.

v5:
  * rebase series now that rte_thread_set_name has been merged,
    remove now unnecessary casts.
  * combine patch adding rte_control_thread_create and patch
    adding unit test into patch 1.
  * reword deprecation notice to indicate which release we
    intend to mark rte_ctrl_thread_create deprecated and which
    release we intend to remove the same.
  * adjust deprecation notice commit subject based on previous
    deprecation notice series feedback.
  * add (mostly unrelated) patch to series to add thread / rte_thread.h
    to doxygen index (an oversight missed from previous series).

v4:
  * fix missing whitespace in deprecation notice text
  * remove comment in rte_control_thread_create implementation
    referring to sched_yield as requested by community feedback
  * add missing parameter name to function pointer declaration

v3:
  * use {ctrl,control}_start_routine for start_routine field names
  * fix conditional evaluation style p == NULL instead of !p
  * tweak documentation comment for rte_control_thread_create
    - "the EAL threads are then excluded"
    - note RTE_MAX_THREAD_NAME_LEN preprocessor definition
      is the name size (including terminating NUL) limit
  * add missing cast to uintptr_t

v2:
  * correct style error void * (*foo) -> void *(*foo)
  * place retval on lhs of comparison 0 != foo() -> foo() != 0
  * add missing commit description on patch 3/3
  * add cast uintptr_t to pthread_t where appropriate
  * fix doxygen @param names to match parameter names

Tyler Retzlaff (3):
  eal: add rte thread create control API
  doc: add missing index entry for thread
  doc: announce deprecation of thread ctrl create function

 app/test/test_threads.c              | 26 +++++++++++
 doc/api/doxy-api-index.md            |  3 +-
 doc/guides/rel_notes/deprecation.rst |  8 ++--
 lib/eal/common/eal_common_thread.c   | 85 ++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h         | 33 ++++++++++++++
 lib/eal/version.map                  |  1 +
 6 files changed, 143 insertions(+), 13 deletions(-)

-- 
1.8.3.1

[PATCH v6 1/3] eal: add rte thread create control API

[Tyler Retzlaff]

Add rte_thread_create_control API as a replacement for
rte_ctrl_thread_create to allow deprecation of the use of platform
specific types in DPDK public API.

Add test from David Marchand to exercise the new API.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 app/test/test_threads.c            | 26 ++++++++++++
 lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 33 +++++++++++++++
 lib/eal/version.map                |  1 +
 4 files changed, 137 insertions(+), 8 deletions(-)

diff --git a/app/test/test_threads.c b/app/test/test_threads.c
index e0f18e4..657ecad 100644
--- a/app/test/test_threads.c
+++ b/app/test/test_threads.c
@@ -232,6 +232,31 @@
 	return 0;
 }
 
+static int
+test_thread_create_control_join(void)
+{
+	rte_thread_t thread_id;
+	rte_thread_t thread_main_id;
+
+	thread_id_ready = 0;
+	RTE_TEST_ASSERT(rte_thread_create_control(&thread_id, "test_control_threads",
+		NULL, thread_main, &thread_main_id) == 0,
+		"Failed to create thread.");
+
+	while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) == 0)
+		;
+
+	RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) != 0,
+		"Unexpected thread id.");
+
+	__atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE);
+
+	RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) == 0,
+		"Failed to join thread.");
+
+	return 0;
+}
+
 static struct unit_test_suite threads_test_suite = {
 	.suite_name = "threads autotest",
 	.setup = NULL,
@@ -243,6 +268,7 @@
 		TEST_CASE(test_thread_priority),
 		TEST_CASE(test_thread_attributes_affinity),
 		TEST_CASE(test_thread_attributes_priority),
+		TEST_CASE(test_thread_create_control_join),
 		TEST_CASES_END()
 	}
 };
diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 3181515..4f83c97 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void *(*ctrl_start_routine)(void *arg);
+		rte_thread_func control_start_routine;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -241,27 +244,47 @@ struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.ctrl_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.control_start_routine;
+
+	if (ctrl_thread_init(arg) != 0)
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.ctrl_start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_thread_create_control(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (params == NULL)
+		return -ENOMEM;
+
+	params->u.control_start_routine = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL)
+		rte_thread_set_name(*thread, name);
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index d247930..fae26a7 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the EAL threads are then
+ * excluded. If setting the name of the thread fails, the error is
+ * ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread
+ *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
+ * @param thread_attr
+ *   Attributes for the new thread.
+ * @param thread_func
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_thread_create_control(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
  * Waits for the thread identified by 'thread_id' to terminate
  *
  * @param thread_id
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 6523102..285a0bd 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -441,6 +441,7 @@ EXPERIMENTAL {
 	rte_thread_join;
 
 	# added in 23.03
+	rte_thread_create_control;
 	rte_thread_set_name;
 };
 
-- 
1.8.3.1

[PATCH v6 2/3] doc: add missing index entry for thread

[Tyler Retzlaff]

Add missing thread index referencing rte_thread.h under the basic
topic.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 doc/api/doxy-api-index.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
index de488c7..2deec7e 100644
--- a/doc/api/doxy-api-index.md
+++ b/doc/api/doxy-api-index.md
@@ -210,7 +210,8 @@ The public API headers are grouped by topics:
   [random](@ref rte_random.h),
   [config file](@ref rte_cfgfile.h),
   [key/value args](@ref rte_kvargs.h),
-  [string](@ref rte_string_fns.h)
+  [string](@ref rte_string_fns.h),
+  [thread](@ref rte_thread.h)
 
 - **debug**:
   [jobstats](@ref rte_jobstats.h),
-- 
1.8.3.1

[PATCH v6 3/3] doc: announce deprecation of thread ctrl create function

[Tyler Retzlaff]

Notify deprecation of rte_ctrl_thread_create API, it will be removed as
it exposes platform-specific thread details.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: David Marchand <david.marchand@redhat.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 doc/guides/rel_notes/deprecation.rst | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ac9aa76..6eb9721 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -23,10 +23,10 @@ Deprecation Notices
   in the future. Applications can use ``devtools/cocci/func_or_ret.cocci``
   to update their code.
 
-* eal: The function ``rte_thread_setname`` is planned to be deprecated
-  starting with the 23.07 release, subject to the replacement API
-  rte_thread_set_name being marked as stable, and planned to be removed
-  by the 23.11 release.
+* eal: The function ``rte_thread_setname`` and ``rte_ctrl_thread_create``
+  are planned to be deprecated starting with the 23.07 release, subject to
+  the replacement API rte_thread_set_name and rte_thread_create_control being
+  marked as stable, and planned to be removed by the 23.11 release.
 
 * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
   not allow for writing optimized code for all the CPU architectures supported
-- 
1.8.3.1

Re: [PATCH v6 0/3] eal: deprecate last use of pthread_t in public API

[David Marchand]

On Wed, Feb 8, 2023 at 10:26 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Announce deprecation of rte_ctrl_thread_create API which is the final
> remaining stable API exposing pthread_t.
>
> Provide an equivalent replacement API rte_thread_create_control that
> uses the new rte_thread_t type.
>
> Provide a unit test for the new rte_thread_create_control API
> (test code provided by David Marchand.)
>
> Add missing doxygen index for thread / rte_thread.h.
>
> Notice!
>
> To limit compatibility regression risk and ease removal of the
> existing rte_ctrl_thread_create in the future duplicate most of the
> existing implementation rather than try to have it accommodate both
> public API contracts.
>
> The duplication, the union introduced to support it along with remaining
> internal pthread_xxx calls will be removed when rte_ctrl_thread_create is
> finally removed.
>
> The old unit test for rte_ctrl_thread_create is kept in place to guarantee
> correct behavior while deprecated and will be removed when
> rte_ctrl_thread_create is finally removed.
>
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>
> v6:
>   * fix missing comma in doxygen index
>   * combine deprecation notice for rte_ctrl_thread_create into notice
>     of removal of rte_thread_setname.
>   * remove test_lcore.c test and add test provided by David Marchand
>     to test_threads.c.
>   * rename the function to start with rte_thread to be consistent
>     with the rest of functions in the rte_thread.h APIs.

Thank you Tyler.

The series lgtm.
The deprecation notice is missing one ack, I'll still wait a bit.
If we don't get it soon, I'll apply the first two patches for rc1.


-- 
David Marchand

Re: [PATCH v6 3/3] doc: announce deprecation of thread ctrl create function

[Bruce Richardson]

On Wed, Feb 08, 2023 at 01:26:35PM -0800, Tyler Retzlaff wrote:
> Notify deprecation of rte_ctrl_thread_create API, it will be removed as
> it exposes platform-specific thread details.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v6 0/3] eal: deprecate last use of pthread_t in public API

[David Marchand]

On Wed, Feb 8, 2023 at 10:26 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Announce deprecation of rte_ctrl_thread_create API which is the final
> remaining stable API exposing pthread_t.
>
> Provide an equivalent replacement API rte_thread_create_control that
> uses the new rte_thread_t type.
>
> Provide a unit test for the new rte_thread_create_control API
> (test code provided by David Marchand.)
>
> Add missing doxygen index for thread / rte_thread.h.
>
> Notice!
>
> To limit compatibility regression risk and ease removal of the
> existing rte_ctrl_thread_create in the future duplicate most of the
> existing implementation rather than try to have it accommodate both
> public API contracts.
>
> The duplication, the union introduced to support it along with remaining
> internal pthread_xxx calls will be removed when rte_ctrl_thread_create is
> finally removed.
>
> The old unit test for rte_ctrl_thread_create is kept in place to guarantee
> correct behavior while deprecated and will be removed when
> rte_ctrl_thread_create is finally removed.
>
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>
> v6:
>   * fix missing comma in doxygen index
>   * combine deprecation notice for rte_ctrl_thread_create into notice
>     of removal of rte_thread_setname.
>   * remove test_lcore.c test and add test provided by David Marchand
>     to test_threads.c.
>   * rename the function to start with rte_thread to be consistent
>     with the rest of functions in the rte_thread.h APIs.
>
> v5:
>   * rebase series now that rte_thread_set_name has been merged,
>     remove now unnecessary casts.
>   * combine patch adding rte_control_thread_create and patch
>     adding unit test into patch 1.
>   * reword deprecation notice to indicate which release we
>     intend to mark rte_ctrl_thread_create deprecated and which
>     release we intend to remove the same.
>   * adjust deprecation notice commit subject based on previous
>     deprecation notice series feedback.
>   * add (mostly unrelated) patch to series to add thread / rte_thread.h
>     to doxygen index (an oversight missed from previous series).
>
> v4:
>   * fix missing whitespace in deprecation notice text
>   * remove comment in rte_control_thread_create implementation
>     referring to sched_yield as requested by community feedback
>   * add missing parameter name to function pointer declaration
>
> v3:
>   * use {ctrl,control}_start_routine for start_routine field names
>   * fix conditional evaluation style p == NULL instead of !p
>   * tweak documentation comment for rte_control_thread_create
>     - "the EAL threads are then excluded"
>     - note RTE_MAX_THREAD_NAME_LEN preprocessor definition
>       is the name size (including terminating NUL) limit
>   * add missing cast to uintptr_t
>
> v2:
>   * correct style error void * (*foo) -> void *(*foo)
>   * place retval on lhs of comparison 0 != foo() -> foo() != 0
>   * add missing commit description on patch 3/3
>   * add cast uintptr_t to pthread_t where appropriate
>   * fix doxygen @param names to match parameter names
>
> Tyler Retzlaff (3):
>   eal: add rte thread create control API
>   doc: add missing index entry for thread
>   doc: announce deprecation of thread ctrl create function
>
>  app/test/test_threads.c              | 26 +++++++++++
>  doc/api/doxy-api-index.md            |  3 +-
>  doc/guides/rel_notes/deprecation.rst |  8 ++--
>  lib/eal/common/eal_common_thread.c   | 85 ++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h         | 33 ++++++++++++++
>  lib/eal/version.map                  |  1 +
>  6 files changed, 143 insertions(+), 13 deletions(-)

Series applied.
Thanks Tyler.


-- 
David Marchand

RE: [PATCH v6 1/3] eal: add rte thread create control API

[Li, WeiyuanX]

Hi, Tyler Retzlaff

We found an error when ASAN test, AddressSanitizer: stack-buffer-overflow error when quit testpmd.
Could you please have a look at it , also submitted a Bugzilla ticket: https://bugs.dpdk.org/show_bug.cgi?id=1166
Thanks.


Regards,
Li, Weiyuan

> -----Original Message-----
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Sent: Thursday, February 9, 2023 5:27 AM
> To: dev@dpdk.org
> Cc: david.marchand@redhat.com; thomas@monjalon.net; Matz, Olivier
> <olivier.matz@6wind.com>; stephen@networkplumber.org;
> mb@smartsharesystems.com; hofors@lysator.liu.se; Tyler Retzlaff
> <roretzla@linux.microsoft.com>
> Subject: [PATCH v6 1/3] eal: add rte thread create control API
> 
> Add rte_thread_create_control API as a replacement for
> rte_ctrl_thread_create to allow deprecation of the use of platform specific
> types in DPDK public API.
> 
> Add test from David Marchand to exercise the new API.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
>  app/test/test_threads.c            | 26 ++++++++++++
>  lib/eal/common/eal_common_thread.c | 85
> ++++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h       | 33 +++++++++++++++
>  lib/eal/version.map                |  1 +
>  4 files changed, 137 insertions(+), 8 deletions(-)
> 
> diff --git a/app/test/test_threads.c b/app/test/test_threads.c index
> e0f18e4..657ecad 100644
> --- a/app/test/test_threads.c
> +++ b/app/test/test_threads.c
> @@ -232,6 +232,31 @@
>  	return 0;
>  }
> 
> +static int
> +test_thread_create_control_join(void)
> +{
> +	rte_thread_t thread_id;
> +	rte_thread_t thread_main_id;
> +
> +	thread_id_ready = 0;
> +	RTE_TEST_ASSERT(rte_thread_create_control(&thread_id,
> "test_control_threads",
> +		NULL, thread_main, &thread_main_id) == 0,
> +		"Failed to create thread.");
> +
> +	while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE)
> == 0)
> +		;
> +
> +	RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) !=
> 0,
> +		"Unexpected thread id.");
> +
> +	__atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE);
> +
> +	RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) == 0,
> +		"Failed to join thread.");
> +
> +	return 0;
> +}
> +
>  static struct unit_test_suite threads_test_suite = {
>  	.suite_name = "threads autotest",
>  	.setup = NULL,
> @@ -243,6 +268,7 @@
>  		TEST_CASE(test_thread_priority),
>  		TEST_CASE(test_thread_attributes_affinity),
>  		TEST_CASE(test_thread_attributes_priority),
> +		TEST_CASE(test_thread_create_control_join),
>  		TEST_CASES_END()
>  	}
>  };
> diff --git a/lib/eal/common/eal_common_thread.c
> b/lib/eal/common/eal_common_thread.c
> index 3181515..4f83c97 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status {  };
> 
>  struct rte_thread_ctrl_params {
> -	void *(*start_routine)(void *);
> +	union {
> +		void *(*ctrl_start_routine)(void *arg);
> +		rte_thread_func control_start_routine;
> +	} u;
>  	void *arg;
>  	int ret;
>  	/* Control thread status.
> @@ -241,27 +244,47 @@ struct rte_thread_ctrl_params {
>  	enum __rte_ctrl_thread_status ctrl_thread_status;  };
> 
> -static void *ctrl_thread_init(void *arg)
> +static int ctrl_thread_init(void *arg)
>  {
>  	struct internal_config *internal_conf =
>  		eal_get_internal_configuration();
>  	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>  	struct rte_thread_ctrl_params *params = arg;
> -	void *(*start_routine)(void *) = params->start_routine;
> -	void *routine_arg = params->arg;
> 
>  	__rte_thread_init(rte_lcore_id(), cpuset);
>  	params->ret = rte_thread_set_affinity_by_id(rte_thread_self(),
> cpuset);
>  	if (params->ret != 0) {
>  		__atomic_store_n(&params->ctrl_thread_status,
>  			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> -		return NULL;
> +		return params->ret;
>  	}
> 
>  	__atomic_store_n(&params->ctrl_thread_status,
>  		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
> 
> -	return start_routine(routine_arg);
> +	return 0;
> +}
> +
> +static void *ctrl_thread_start(void *arg) {
> +	struct rte_thread_ctrl_params *params = arg;
> +	void *(*start_routine)(void *) = params->u.ctrl_start_routine;
> +
> +	if (ctrl_thread_init(arg) != 0)
> +		return NULL;
> +
> +	return start_routine(params->arg);
> +}
> +
> +static uint32_t control_thread_start(void *arg) {
> +	struct rte_thread_ctrl_params *params = arg;
> +	rte_thread_func start_routine = params->u.control_start_routine;
> +
> +	if (ctrl_thread_init(arg) != 0)
> +		return params->ret;
> +
> +	return start_routine(params->arg);
>  }
> 
>  int
> @@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg)
>  	if (!params)
>  		return -ENOMEM;
> 
> -	params->start_routine = start_routine;
> +	params->u.ctrl_start_routine = start_routine;
>  	params->arg = arg;
>  	params->ret = 0;
>  	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> 
> -	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> +	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
>  	if (ret != 0) {
>  		free(params);
>  		return -ret;
> @@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg)  }
> 
>  int
> +rte_thread_create_control(rte_thread_t *thread, const char *name,
> +	const rte_thread_attr_t *attr,
> +	rte_thread_func start_routine, void *arg) {
> +	struct rte_thread_ctrl_params *params;
> +	enum __rte_ctrl_thread_status ctrl_thread_status;
> +	int ret;
> +
> +	params = malloc(sizeof(*params));
> +	if (params == NULL)
> +		return -ENOMEM;
> +
> +	params->u.control_start_routine = start_routine;
> +	params->arg = arg;
> +	params->ret = 0;
> +	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> +
> +	ret = rte_thread_create(thread, attr, control_thread_start, params);
> +	if (ret != 0) {
> +		free(params);
> +		return -ret;
> +	}
> +
> +	if (name != NULL)
> +		rte_thread_set_name(*thread, name);
> +
> +	/* Wait for the control thread to initialize successfully */
> +	while ((ctrl_thread_status =
> +			__atomic_load_n(&params->ctrl_thread_status,
> +			__ATOMIC_ACQUIRE)) ==
> CTRL_THREAD_LAUNCHING) {
> +		rte_delay_us_sleep(1);
> +	}
> +
> +	/* Check if the control thread encountered an error */
> +	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
> +		/* ctrl thread is exiting */
> +		rte_thread_join(*thread, NULL);
> +	}
> +
> +	ret = params->ret;
> +	free(params);
> +
> +	return ret;
> +}
> +
> +int
>  rte_thread_register(void)
>  {
>  	unsigned int lcore_id;
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h index
> d247930..fae26a7 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id,
>   * @warning
>   * @b EXPERIMENTAL: this API may change without prior notice.
>   *
> + * Create a control thread.
> + *
> + * Creates a control thread with the given name and attributes. The
> + * affinity of the new thread is based on the CPU affinity retrieved
> + * at the time rte_eal_init() was called, the EAL threads are then
> + * excluded. If setting the name of the thread fails, the error is
> + * ignored and a debug message is logged.
> + *
> + * @param thread
> + *   Filled with the thread id of the new created thread.
> + * @param name
> + *   The name of the control thread
> + *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
> + * @param thread_attr
> + *   Attributes for the new thread.
> + * @param thread_func
> + *   Function to be executed by the new thread.
> + * @param arg
> + *   Argument passed to start_routine.
> + * @return
> + *   On success, returns 0; on error, it returns a negative value
> + *   corresponding to the error number.
> + */
> +__rte_experimental
> +int
> +rte_thread_create_control(rte_thread_t *thread, const char *name,
> +		const rte_thread_attr_t *thread_attr,
> +		rte_thread_func thread_func, void *arg);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
>   * Waits for the thread identified by 'thread_id' to terminate
>   *
>   * @param thread_id
> diff --git a/lib/eal/version.map b/lib/eal/version.map index 6523102..285a0bd
> 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -441,6 +441,7 @@ EXPERIMENTAL {
>  	rte_thread_join;
> 
>  	# added in 23.03
> +	rte_thread_create_control;
>  	rte_thread_set_name;
>  };
> 
> --
> 1.8.3.1

Re: [PATCH v6 1/3] eal: add rte thread create control API

[David Marchand]

On Wed, Feb 8, 2023 at 10:26 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Add rte_thread_create_control API as a replacement for
> rte_ctrl_thread_create to allow deprecation of the use of platform
> specific types in DPDK public API.
>
> Add test from David Marchand to exercise the new API.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
>  app/test/test_threads.c            | 26 ++++++++++++
>  lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h       | 33 +++++++++++++++
>  lib/eal/version.map                |  1 +
>  4 files changed, 137 insertions(+), 8 deletions(-)
>
> diff --git a/app/test/test_threads.c b/app/test/test_threads.c
> index e0f18e4..657ecad 100644
> --- a/app/test/test_threads.c
> +++ b/app/test/test_threads.c
> @@ -232,6 +232,31 @@
>         return 0;
>  }
>
> +static int
> +test_thread_create_control_join(void)
> +{
> +       rte_thread_t thread_id;
> +       rte_thread_t thread_main_id;
> +
> +       thread_id_ready = 0;
> +       RTE_TEST_ASSERT(rte_thread_create_control(&thread_id, "test_control_threads",
> +               NULL, thread_main, &thread_main_id) == 0,
> +               "Failed to create thread.");
> +
> +       while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) == 0)
> +               ;
> +
> +       RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) != 0,
> +               "Unexpected thread id.");
> +
> +       __atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE);
> +
> +       RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) == 0,
> +               "Failed to join thread.");
> +
> +       return 0;
> +}
> +
>  static struct unit_test_suite threads_test_suite = {
>         .suite_name = "threads autotest",
>         .setup = NULL,
> @@ -243,6 +268,7 @@
>                 TEST_CASE(test_thread_priority),
>                 TEST_CASE(test_thread_attributes_affinity),
>                 TEST_CASE(test_thread_attributes_priority),
> +               TEST_CASE(test_thread_create_control_join),
>                 TEST_CASES_END()
>         }
>  };
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 3181515..4f83c97 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status {
>  };
>
>  struct rte_thread_ctrl_params {
> -       void *(*start_routine)(void *);
> +       union {
> +               void *(*ctrl_start_routine)(void *arg);
> +               rte_thread_func control_start_routine;
> +       } u;
>         void *arg;
>         int ret;
>         /* Control thread status.
> @@ -241,27 +244,47 @@ struct rte_thread_ctrl_params {
>         enum __rte_ctrl_thread_status ctrl_thread_status;
>  };
>
> -static void *ctrl_thread_init(void *arg)
> +static int ctrl_thread_init(void *arg)
>  {
>         struct internal_config *internal_conf =
>                 eal_get_internal_configuration();
>         rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>         struct rte_thread_ctrl_params *params = arg;
> -       void *(*start_routine)(void *) = params->start_routine;
> -       void *routine_arg = params->arg;
>
>         __rte_thread_init(rte_lcore_id(), cpuset);
>         params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
>         if (params->ret != 0) {
>                 __atomic_store_n(&params->ctrl_thread_status,
>                         CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> -               return NULL;
> +               return params->ret;
>         }
>
>         __atomic_store_n(&params->ctrl_thread_status,
>                 CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
>
> -       return start_routine(routine_arg);
> +       return 0;
> +}
> +
> +static void *ctrl_thread_start(void *arg)
> +{
> +       struct rte_thread_ctrl_params *params = arg;
> +       void *(*start_routine)(void *) = params->u.ctrl_start_routine;
> +
> +       if (ctrl_thread_init(arg) != 0)
> +               return NULL;
> +
> +       return start_routine(params->arg);

Does this patch open a race on params->arg between the newly created
thread, and the one that asked for creation?
Reading params->arg is not under the ctrl_thread_status
syncrhonisation point anymore (which was protecting against
free(params)).

WDYT?

(Note that I doubt this patch has anything to do with the recent bz
opened by Intel QE)


> +}
> +
> +static uint32_t control_thread_start(void *arg)
> +{
> +       struct rte_thread_ctrl_params *params = arg;
> +       rte_thread_func start_routine = params->u.control_start_routine;
> +
> +       if (ctrl_thread_init(arg) != 0)
> +               return params->ret;

Idem here for params->ret.


> +
> +       return start_routine(params->arg);
>  }
>
>  int
> @@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg)
>         if (!params)
>                 return -ENOMEM;
>
> -       params->start_routine = start_routine;
> +       params->u.ctrl_start_routine = start_routine;
>         params->arg = arg;
>         params->ret = 0;
>         params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>
> -       ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> +       ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
>         if (ret != 0) {
>                 free(params);
>                 return -ret;
> @@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg)
>  }
>
>  int
> +rte_thread_create_control(rte_thread_t *thread, const char *name,
> +       const rte_thread_attr_t *attr,
> +       rte_thread_func start_routine, void *arg)
> +{
> +       struct rte_thread_ctrl_params *params;
> +       enum __rte_ctrl_thread_status ctrl_thread_status;
> +       int ret;
> +
> +       params = malloc(sizeof(*params));
> +       if (params == NULL)
> +               return -ENOMEM;
> +
> +       params->u.control_start_routine = start_routine;
> +       params->arg = arg;
> +       params->ret = 0;
> +       params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> +
> +       ret = rte_thread_create(thread, attr, control_thread_start, params);
> +       if (ret != 0) {
> +               free(params);
> +               return -ret;
> +       }
> +
> +       if (name != NULL)
> +               rte_thread_set_name(*thread, name);
> +
> +       /* Wait for the control thread to initialize successfully */
> +       while ((ctrl_thread_status =
> +                       __atomic_load_n(&params->ctrl_thread_status,
> +                       __ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> +               rte_delay_us_sleep(1);
> +       }
> +
> +       /* Check if the control thread encountered an error */
> +       if (ctrl_thread_status == CTRL_THREAD_ERROR) {
> +               /* ctrl thread is exiting */
> +               rte_thread_join(*thread, NULL);
> +       }
> +
> +       ret = params->ret;
> +       free(params);
> +
> +       return ret;
> +}
> +
> +int
>  rte_thread_register(void)
>  {
>         unsigned int lcore_id;
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index d247930..fae26a7 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id,
>   * @warning
>   * @b EXPERIMENTAL: this API may change without prior notice.
>   *
> + * Create a control thread.
> + *
> + * Creates a control thread with the given name and attributes. The
> + * affinity of the new thread is based on the CPU affinity retrieved
> + * at the time rte_eal_init() was called, the EAL threads are then
> + * excluded. If setting the name of the thread fails, the error is
> + * ignored and a debug message is logged.
> + *
> + * @param thread
> + *   Filled with the thread id of the new created thread.
> + * @param name
> + *   The name of the control thread
> + *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
> + * @param thread_attr
> + *   Attributes for the new thread.
> + * @param thread_func
> + *   Function to be executed by the new thread.
> + * @param arg
> + *   Argument passed to start_routine.
> + * @return
> + *   On success, returns 0; on error, it returns a negative value
> + *   corresponding to the error number.
> + */
> +__rte_experimental
> +int
> +rte_thread_create_control(rte_thread_t *thread, const char *name,
> +               const rte_thread_attr_t *thread_attr,
> +               rte_thread_func thread_func, void *arg);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
>   * Waits for the thread identified by 'thread_id' to terminate
>   *
>   * @param thread_id
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index 6523102..285a0bd 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -441,6 +441,7 @@ EXPERIMENTAL {
>         rte_thread_join;
>
>         # added in 23.03
> +       rte_thread_create_control;
>         rte_thread_set_name;
>  };
>
> --
> 1.8.3.1
>


-- 
David Marchand

Re: [PATCH v6 1/3] eal: add rte thread create control API

[Tyler Retzlaff]

On Fri, Feb 24, 2023 at 09:13:56AM +0100, David Marchand wrote:
> On Wed, Feb 8, 2023 at 10:26 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Add rte_thread_create_control API as a replacement for
> > rte_ctrl_thread_create to allow deprecation of the use of platform
> > specific types in DPDK public API.
> >
> > Add test from David Marchand to exercise the new API.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > ---
> >  app/test/test_threads.c            | 26 ++++++++++++
> >  lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 33 +++++++++++++++
> >  lib/eal/version.map                |  1 +
> >  4 files changed, 137 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test/test_threads.c b/app/test/test_threads.c
> > index e0f18e4..657ecad 100644
> > --- a/app/test/test_threads.c
> > +++ b/app/test/test_threads.c
> > @@ -232,6 +232,31 @@
> >         return 0;
> >  }
> >
> > +static int
> > +test_thread_create_control_join(void)
> > +{
> > +       rte_thread_t thread_id;
> > +       rte_thread_t thread_main_id;
> > +
> > +       thread_id_ready = 0;
> > +       RTE_TEST_ASSERT(rte_thread_create_control(&thread_id, "test_control_threads",
> > +               NULL, thread_main, &thread_main_id) == 0,
> > +               "Failed to create thread.");
> > +
> > +       while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) == 0)
> > +               ;
> > +
> > +       RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) != 0,
> > +               "Unexpected thread id.");
> > +
> > +       __atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE);
> > +
> > +       RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) == 0,
> > +               "Failed to join thread.");
> > +
> > +       return 0;
> > +}
> > +
> >  static struct unit_test_suite threads_test_suite = {
> >         .suite_name = "threads autotest",
> >         .setup = NULL,
> > @@ -243,6 +268,7 @@
> >                 TEST_CASE(test_thread_priority),
> >                 TEST_CASE(test_thread_attributes_affinity),
> >                 TEST_CASE(test_thread_attributes_priority),
> > +               TEST_CASE(test_thread_create_control_join),
> >                 TEST_CASES_END()
> >         }
> >  };
> > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > index 3181515..4f83c97 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >
> >  struct rte_thread_ctrl_params {
> > -       void *(*start_routine)(void *);
> > +       union {
> > +               void *(*ctrl_start_routine)(void *arg);
> > +               rte_thread_func control_start_routine;
> > +       } u;
> >         void *arg;
> >         int ret;
> >         /* Control thread status.
> > @@ -241,27 +244,47 @@ struct rte_thread_ctrl_params {
> >         enum __rte_ctrl_thread_status ctrl_thread_status;
> >  };
> >
> > -static void *ctrl_thread_init(void *arg)
> > +static int ctrl_thread_init(void *arg)
> >  {
> >         struct internal_config *internal_conf =
> >                 eal_get_internal_configuration();
> >         rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
> >         struct rte_thread_ctrl_params *params = arg;
> > -       void *(*start_routine)(void *) = params->start_routine;
> > -       void *routine_arg = params->arg;
> >
> >         __rte_thread_init(rte_lcore_id(), cpuset);
> >         params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
> >         if (params->ret != 0) {
> >                 __atomic_store_n(&params->ctrl_thread_status,
> >                         CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> > -               return NULL;
> > +               return params->ret;
> >         }
> >
> >         __atomic_store_n(&params->ctrl_thread_status,
> >                 CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
> >
> > -       return start_routine(routine_arg);
> > +       return 0;
> > +}
> > +
> > +static void *ctrl_thread_start(void *arg)
> > +{
> > +       struct rte_thread_ctrl_params *params = arg;
> > +       void *(*start_routine)(void *) = params->u.ctrl_start_routine;
> > +
> > +       if (ctrl_thread_init(arg) != 0)
> > +               return NULL;
> > +
> > +       return start_routine(params->arg);
> 
> Does this patch open a race on params->arg between the newly created
> thread, and the one that asked for creation?
> Reading params->arg is not under the ctrl_thread_status
> syncrhonisation point anymore (which was protecting against
> free(params)).
> 
> WDYT?

i will take a look, thanks for letting me know.

> 
> (Note that I doubt this patch has anything to do with the recent bz
> opened by Intel QE)
> 
> 
> > +}
> > +
> > +static uint32_t control_thread_start(void *arg)
> > +{
> > +       struct rte_thread_ctrl_params *params = arg;
> > +       rte_thread_func start_routine = params->u.control_start_routine;
> > +
> > +       if (ctrl_thread_init(arg) != 0)
> > +               return params->ret;
> 
> Idem here for params->ret.
> 
> 
> > +
> > +       return start_routine(params->arg);
> >  }
> >
> >  int
> > @@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg)
> >         if (!params)
> >                 return -ENOMEM;
> >
> > -       params->start_routine = start_routine;
> > +       params->u.ctrl_start_routine = start_routine;
> >         params->arg = arg;
> >         params->ret = 0;
> >         params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> >
> > -       ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> > +       ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
> >         if (ret != 0) {
> >                 free(params);
> >                 return -ret;
> > @@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg)
> >  }
> >
> >  int
> > +rte_thread_create_control(rte_thread_t *thread, const char *name,
> > +       const rte_thread_attr_t *attr,
> > +       rte_thread_func start_routine, void *arg)
> > +{
> > +       struct rte_thread_ctrl_params *params;
> > +       enum __rte_ctrl_thread_status ctrl_thread_status;
> > +       int ret;
> > +
> > +       params = malloc(sizeof(*params));
> > +       if (params == NULL)
> > +               return -ENOMEM;
> > +
> > +       params->u.control_start_routine = start_routine;
> > +       params->arg = arg;
> > +       params->ret = 0;
> > +       params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> > +
> > +       ret = rte_thread_create(thread, attr, control_thread_start, params);
> > +       if (ret != 0) {
> > +               free(params);
> > +               return -ret;
> > +       }
> > +
> > +       if (name != NULL)
> > +               rte_thread_set_name(*thread, name);
> > +
> > +       /* Wait for the control thread to initialize successfully */
> > +       while ((ctrl_thread_status =
> > +                       __atomic_load_n(&params->ctrl_thread_status,
> > +                       __ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> > +               rte_delay_us_sleep(1);
> > +       }
> > +
> > +       /* Check if the control thread encountered an error */
> > +       if (ctrl_thread_status == CTRL_THREAD_ERROR) {
> > +               /* ctrl thread is exiting */
> > +               rte_thread_join(*thread, NULL);
> > +       }
> > +
> > +       ret = params->ret;
> > +       free(params);
> > +
> > +       return ret;
> > +}
> > +
> > +int
> >  rte_thread_register(void)
> >  {
> >         unsigned int lcore_id;
> > diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> > index d247930..fae26a7 100644
> > --- a/lib/eal/include/rte_thread.h
> > +++ b/lib/eal/include/rte_thread.h
> > @@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id,
> >   * @warning
> >   * @b EXPERIMENTAL: this API may change without prior notice.
> >   *
> > + * Create a control thread.
> > + *
> > + * Creates a control thread with the given name and attributes. The
> > + * affinity of the new thread is based on the CPU affinity retrieved
> > + * at the time rte_eal_init() was called, the EAL threads are then
> > + * excluded. If setting the name of the thread fails, the error is
> > + * ignored and a debug message is logged.
> > + *
> > + * @param thread
> > + *   Filled with the thread id of the new created thread.
> > + * @param name
> > + *   The name of the control thread
> > + *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
> > + * @param thread_attr
> > + *   Attributes for the new thread.
> > + * @param thread_func
> > + *   Function to be executed by the new thread.
> > + * @param arg
> > + *   Argument passed to start_routine.
> > + * @return
> > + *   On success, returns 0; on error, it returns a negative value
> > + *   corresponding to the error number.
> > + */
> > +__rte_experimental
> > +int
> > +rte_thread_create_control(rte_thread_t *thread, const char *name,
> > +               const rte_thread_attr_t *thread_attr,
> > +               rte_thread_func thread_func, void *arg);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> >   * Waits for the thread identified by 'thread_id' to terminate
> >   *
> >   * @param thread_id
> > diff --git a/lib/eal/version.map b/lib/eal/version.map
> > index 6523102..285a0bd 100644
> > --- a/lib/eal/version.map
> > +++ b/lib/eal/version.map
> > @@ -441,6 +441,7 @@ EXPERIMENTAL {
> >         rte_thread_join;
> >
> >         # added in 23.03
> > +       rte_thread_create_control;
> >         rte_thread_set_name;
> >  };
> >
> > --
> > 1.8.3.1
> >
> 
> 
> -- 
> David Marchand

Re: [PATCH v6 1/3] eal: add rte thread create control API

[Tyler Retzlaff]

On Fri, Feb 24, 2023 at 09:13:56AM +0100, David Marchand wrote:
> On Wed, Feb 8, 2023 at 10:26 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Add rte_thread_create_control API as a replacement for
> > rte_ctrl_thread_create to allow deprecation of the use of platform
> > specific types in DPDK public API.
> >
> > Add test from David Marchand to exercise the new API.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > ---
> >  app/test/test_threads.c            | 26 ++++++++++++
> >  lib/eal/common/eal_common_thread.c | 85 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 33 +++++++++++++++
> >  lib/eal/version.map                |  1 +
> >  4 files changed, 137 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test/test_threads.c b/app/test/test_threads.c
> > index e0f18e4..657ecad 100644
> > --- a/app/test/test_threads.c
> > +++ b/app/test/test_threads.c
> > @@ -232,6 +232,31 @@
> >         return 0;
> >  }
> >
> > +static int
> > +test_thread_create_control_join(void)
> > +{
> > +       rte_thread_t thread_id;
> > +       rte_thread_t thread_main_id;
> > +
> > +       thread_id_ready = 0;
> > +       RTE_TEST_ASSERT(rte_thread_create_control(&thread_id, "test_control_threads",
> > +               NULL, thread_main, &thread_main_id) == 0,
> > +               "Failed to create thread.");
> > +
> > +       while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) == 0)
> > +               ;
> > +
> > +       RTE_TEST_ASSERT(rte_thread_equal(thread_id, thread_main_id) != 0,
> > +               "Unexpected thread id.");
> > +
> > +       __atomic_store_n(&thread_id_ready, 2, __ATOMIC_RELEASE);
> > +
> > +       RTE_TEST_ASSERT(rte_thread_join(thread_id, NULL) == 0,
> > +               "Failed to join thread.");
> > +
> > +       return 0;
> > +}
> > +
> >  static struct unit_test_suite threads_test_suite = {
> >         .suite_name = "threads autotest",
> >         .setup = NULL,
> > @@ -243,6 +268,7 @@
> >                 TEST_CASE(test_thread_priority),
> >                 TEST_CASE(test_thread_attributes_affinity),
> >                 TEST_CASE(test_thread_attributes_priority),
> > +               TEST_CASE(test_thread_create_control_join),
> >                 TEST_CASES_END()
> >         }
> >  };
> > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > index 3181515..4f83c97 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -232,7 +232,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >
> >  struct rte_thread_ctrl_params {
> > -       void *(*start_routine)(void *);
> > +       union {
> > +               void *(*ctrl_start_routine)(void *arg);
> > +               rte_thread_func control_start_routine;
> > +       } u;
> >         void *arg;
> >         int ret;
> >         /* Control thread status.
> > @@ -241,27 +244,47 @@ struct rte_thread_ctrl_params {
> >         enum __rte_ctrl_thread_status ctrl_thread_status;
> >  };
> >
> > -static void *ctrl_thread_init(void *arg)
> > +static int ctrl_thread_init(void *arg)
> >  {
> >         struct internal_config *internal_conf =
> >                 eal_get_internal_configuration();
> >         rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
> >         struct rte_thread_ctrl_params *params = arg;
> > -       void *(*start_routine)(void *) = params->start_routine;
> > -       void *routine_arg = params->arg;
> >
> >         __rte_thread_init(rte_lcore_id(), cpuset);
> >         params->ret = rte_thread_set_affinity_by_id(rte_thread_self(), cpuset);
> >         if (params->ret != 0) {
> >                 __atomic_store_n(&params->ctrl_thread_status,
> >                         CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> > -               return NULL;
> > +               return params->ret;

here too, though a narrow window.

> >         }
> >
> >         __atomic_store_n(&params->ctrl_thread_status,
> >                 CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
> >
> > -       return start_routine(routine_arg);
> > +       return 0;
> > +}
> > +
> > +static void *ctrl_thread_start(void *arg)
> > +{
> > +       struct rte_thread_ctrl_params *params = arg;
> > +       void *(*start_routine)(void *) = params->u.ctrl_start_routine;
> > +
> > +       if (ctrl_thread_init(arg) != 0)
> > +               return NULL;
> > +
> > +       return start_routine(params->arg);
> 
> Does this patch open a race on params->arg between the newly created
> thread, and the one that asked for creation?
> Reading params->arg is not under the ctrl_thread_status
> syncrhonisation point anymore (which was protecting against
> free(params)).
> 
> WDYT?

yeah, this is definitely wrong. i'm preparing a patch now. both
params->ret and params->arg are loaded unsynchronized in multiple
places.

sorry about the mistake. fix coming soon.

> 
> (Note that I doubt this patch has anything to do with the recent bz
> opened by Intel QE)
> 
> 
> > +}
> > +
> > +static uint32_t control_thread_start(void *arg)
> > +{
> > +       struct rte_thread_ctrl_params *params = arg;
> > +       rte_thread_func start_routine = params->u.control_start_routine;
> > +
> > +       if (ctrl_thread_init(arg) != 0)
> > +               return params->ret;
> 
> Idem here for params->ret.

> 
> 
> > +
> > +       return start_routine(params->arg);
> >  }
> >
> >  int
> > @@ -277,12 +300,12 @@ static void *ctrl_thread_init(void *arg)
> >         if (!params)
> >                 return -ENOMEM;
> >
> > -       params->start_routine = start_routine;
> > +       params->u.ctrl_start_routine = start_routine;
> >         params->arg = arg;
> >         params->ret = 0;
> >         params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> >
> > -       ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> > +       ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
> >         if (ret != 0) {
> >                 free(params);
> >                 return -ret;
> > @@ -315,6 +338,52 @@ static void *ctrl_thread_init(void *arg)
> >  }
> >
> >  int
> > +rte_thread_create_control(rte_thread_t *thread, const char *name,
> > +       const rte_thread_attr_t *attr,
> > +       rte_thread_func start_routine, void *arg)
> > +{
> > +       struct rte_thread_ctrl_params *params;
> > +       enum __rte_ctrl_thread_status ctrl_thread_status;
> > +       int ret;
> > +
> > +       params = malloc(sizeof(*params));
> > +       if (params == NULL)
> > +               return -ENOMEM;
> > +
> > +       params->u.control_start_routine = start_routine;
> > +       params->arg = arg;
> > +       params->ret = 0;
> > +       params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
> > +
> > +       ret = rte_thread_create(thread, attr, control_thread_start, params);
> > +       if (ret != 0) {
> > +               free(params);
> > +               return -ret;
> > +       }
> > +
> > +       if (name != NULL)
> > +               rte_thread_set_name(*thread, name);
> > +
> > +       /* Wait for the control thread to initialize successfully */
> > +       while ((ctrl_thread_status =
> > +                       __atomic_load_n(&params->ctrl_thread_status,
> > +                       __ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> > +               rte_delay_us_sleep(1);
> > +       }
> > +
> > +       /* Check if the control thread encountered an error */
> > +       if (ctrl_thread_status == CTRL_THREAD_ERROR) {
> > +               /* ctrl thread is exiting */
> > +               rte_thread_join(*thread, NULL);
> > +       }
> > +
> > +       ret = params->ret;
> > +       free(params);
> > +
> > +       return ret;
> > +}
> > +
> > +int
> >  rte_thread_register(void)
> >  {
> >         unsigned int lcore_id;
> > diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> > index d247930..fae26a7 100644
> > --- a/lib/eal/include/rte_thread.h
> > +++ b/lib/eal/include/rte_thread.h
> > @@ -98,6 +98,39 @@ int rte_thread_create(rte_thread_t *thread_id,
> >   * @warning
> >   * @b EXPERIMENTAL: this API may change without prior notice.
> >   *
> > + * Create a control thread.
> > + *
> > + * Creates a control thread with the given name and attributes. The
> > + * affinity of the new thread is based on the CPU affinity retrieved
> > + * at the time rte_eal_init() was called, the EAL threads are then
> > + * excluded. If setting the name of the thread fails, the error is
> > + * ignored and a debug message is logged.
> > + *
> > + * @param thread
> > + *   Filled with the thread id of the new created thread.
> > + * @param name
> > + *   The name of the control thread
> > + *   (max RTE_MAX_THREAD_NAME_LEN characters including '\0').
> > + * @param thread_attr
> > + *   Attributes for the new thread.
> > + * @param thread_func
> > + *   Function to be executed by the new thread.
> > + * @param arg
> > + *   Argument passed to start_routine.
> > + * @return
> > + *   On success, returns 0; on error, it returns a negative value
> > + *   corresponding to the error number.
> > + */
> > +__rte_experimental
> > +int
> > +rte_thread_create_control(rte_thread_t *thread, const char *name,
> > +               const rte_thread_attr_t *thread_attr,
> > +               rte_thread_func thread_func, void *arg);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> >   * Waits for the thread identified by 'thread_id' to terminate
> >   *
> >   * @param thread_id
> > diff --git a/lib/eal/version.map b/lib/eal/version.map
> > index 6523102..285a0bd 100644
> > --- a/lib/eal/version.map
> > +++ b/lib/eal/version.map
> > @@ -441,6 +441,7 @@ EXPERIMENTAL {
> >         rte_thread_join;
> >
> >         # added in 23.03
> > +       rte_thread_create_control;
> >         rte_thread_set_name;
> >  };
> >
> > --
> > 1.8.3.1
> >
> 
> 
> -- 
> David Marchand