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

  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(-)
  

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 {
  

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 {
  

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!
  

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 {
  

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!
  

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.
  

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