[dpdk-dev] [RFC] eal: simplify the implementation of rte_ctrl_thread_create

[dpdk-dev] [RFC] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

The current described behaviour of rte_ctrl_thread_create is
rigid which makes the implementation of the function complex.
The behavior is abstracted to allow for simplified implementation.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
 lib/eal/include/rte_lcore.h        |  8 ++--
 2 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 1a52f42a2b..86cacd840c 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -169,35 +169,35 @@ __rte_thread_uninit(void)
 struct rte_thread_ctrl_params {
 	void *(*start_routine)(void *);
 	void *arg;
-	pthread_barrier_t configured;
-	unsigned int refcnt;
+	int ret;
+	/* Synchronization variable between the control thread
+	 * and the thread calling rte_ctrl_thread_create function.
+	 * 0 - Initialized
+	 * 1 - Control thread is running successfully
+	 * 2 - Control thread encountered an error. 'ret' has the
+	 *     error code.
+	 */
+	unsigned int sync;
 };
 
-static void ctrl_params_free(struct rte_thread_ctrl_params *params)
-{
-	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
-		(void)pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
-}
-
 static void *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 *);
+	void *(*start_routine)(void *) = params->start_routine;
 	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
-
-	pthread_barrier_wait(&params->configured);
-	start_routine = params->start_routine;
-	ctrl_params_free(params);
-
-	if (start_routine == NULL)
+	params->ret = pthread_setaffinity_np(pthread_self(),
+				sizeof(*cpuset), cpuset);
+	if (params->ret != 0) {
+		params->sync = 2;
 		return NULL;
+	}
+
+	params->sync = 1;
 
 	return start_routine(routine_arg);
 }
@@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 		const pthread_attr_t *attr,
 		void *(*start_routine)(void *), 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;
 	int ret;
 
@@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params->start_routine = start_routine;
 	params->arg = arg;
-	params->refcnt = 2;
-
-	ret = pthread_barrier_init(&params->configured, NULL, 2);
-	if (ret != 0)
-		goto fail_no_barrier;
+	params->ret = 0;
+	params->sync = 0;
 
 	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
 	if (ret != 0)
-		goto fail_with_barrier;
+		goto thread_create_failed;
 
 	if (name != NULL) {
 		ret = rte_thread_setname(*thread, name);
@@ -236,24 +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 				"Cannot set name for ctrl thread\n");
 	}
 
-	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
-	if (ret != 0)
-		params->start_routine = NULL;
+	/* Wait for the control thread to initialize successfully */
+	while (!params->sync)
+		rte_pause();
+	ret = params->ret;
 
-	pthread_barrier_wait(&params->configured);
-	ctrl_params_free(params);
+	free(params);
 
-	if (ret != 0)
-		/* start_routine has been set to NULL above; */
-		/* ctrl thread will exit immediately */
+	if (params->sync != 1)
+		/* ctrl thread is exiting */
 		pthread_join(*thread, NULL);
 
 	return -ret;
 
-fail_with_barrier:
-	(void)pthread_barrier_destroy(&params->configured);
+thread_create_failed:
 
-fail_no_barrier:
 	free(params);
 
 	return -ret;
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 1550b75da0..f1cc5e38dc 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -420,10 +420,10 @@ rte_thread_unregister(void);
 /**
  * Create a control thread.
  *
- * Wrapper to pthread_create(), pthread_setname_np() and
- * pthread_setaffinity_np(). 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.
+ * 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.
  *
  * @param thread
  *   Filled with the thread id of the new created thread.
-- 
2.17.1

[dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

The current described behaviour of rte_ctrl_thread_create is
rigid which makes the implementation of the function complex.
The behavior is abstracted to allow for simplified implementation.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
v2: Used compiler's C++11 atomic built-ins to access the shared variable.

 lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
 lib/eal/include/rte_lcore.h        |  8 ++--
 2 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 1a52f42a2b..e3e0bf4bff 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -169,35 +169,35 @@ __rte_thread_uninit(void)
 struct rte_thread_ctrl_params {
 	void *(*start_routine)(void *);
 	void *arg;
-	pthread_barrier_t configured;
-	unsigned int refcnt;
+	int ret;
+	/* Synchronization variable between the control thread
+	 * and the thread calling rte_ctrl_thread_create function.
+	 * 0 - Initialized
+	 * 1 - Control thread is running successfully
+	 * 2 - Control thread encountered an error. 'ret' has the
+	 *     error code.
+	 */
+	unsigned int sync;
 };
 
-static void ctrl_params_free(struct rte_thread_ctrl_params *params)
-{
-	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
-		(void)pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
-}
-
 static void *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 *);
+	void *(*start_routine)(void *) = params->start_routine;
 	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
-
-	pthread_barrier_wait(&params->configured);
-	start_routine = params->start_routine;
-	ctrl_params_free(params);
-
-	if (start_routine == NULL)
+	params->ret = pthread_setaffinity_np(pthread_self(),
+				sizeof(*cpuset), cpuset);
+	if (params->ret != 0) {
+		__atomic_store_n(&params->sync, 2, __ATOMIC_RELEASE);
 		return NULL;
+	}
+
+	__atomic_store_n(&params->sync, 1, __ATOMIC_RELEASE);
 
 	return start_routine(routine_arg);
 }
@@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 		const pthread_attr_t *attr,
 		void *(*start_routine)(void *), 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;
 	int ret;
 
@@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params->start_routine = start_routine;
 	params->arg = arg;
-	params->refcnt = 2;
-
-	ret = pthread_barrier_init(&params->configured, NULL, 2);
-	if (ret != 0)
-		goto fail_no_barrier;
+	params->ret = 0;
+	params->sync = 0;
 
 	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
 	if (ret != 0)
-		goto fail_with_barrier;
+		goto thread_create_failed;
 
 	if (name != NULL) {
 		ret = rte_thread_setname(*thread, name);
@@ -236,24 +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 				"Cannot set name for ctrl thread\n");
 	}
 
-	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
-	if (ret != 0)
-		params->start_routine = NULL;
+	/* Wait for the control thread to initialize successfully */
+	while (!__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE))
+		rte_pause();
+	ret = params->ret;
 
-	pthread_barrier_wait(&params->configured);
-	ctrl_params_free(params);
+	free(params);
 
-	if (ret != 0)
-		/* start_routine has been set to NULL above; */
-		/* ctrl thread will exit immediately */
+	if (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) != 1)
+		/* ctrl thread is exiting */
 		pthread_join(*thread, NULL);
 
 	return -ret;
 
-fail_with_barrier:
-	(void)pthread_barrier_destroy(&params->configured);
+thread_create_failed:
 
-fail_no_barrier:
 	free(params);
 
 	return -ret;
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 1550b75da0..f1cc5e38dc 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -420,10 +420,10 @@ rte_thread_unregister(void);
 /**
  * Create a control thread.
  *
- * Wrapper to pthread_create(), pthread_setname_np() and
- * pthread_setaffinity_np(). 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.
+ * 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.
  *
  * @param thread
  *   Filled with the thread id of the new created thread.
-- 
2.17.1

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Olivier Matz]

Hi Honnappa,

On Mon, Aug 02, 2021 at 12:16:52AM -0500, Honnappa Nagarahalli wrote:
> The current described behaviour of rte_ctrl_thread_create is
> rigid which makes the implementation of the function complex.
> The behavior is abstracted to allow for simplified implementation.

I agree that the behavior description should not reference the pthread
functions, however I don't think the current description prevents from
rewriting the code as you do.

I think it would be better to explain in commit log why the proposed
code is simpler than the current one.

> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> v2: Used compiler's C++11 atomic built-ins to access the shared variable.
> 
>  lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
>  lib/eal/include/rte_lcore.h        |  8 ++--
>  2 files changed, 32 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 1a52f42a2b..e3e0bf4bff 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -169,35 +169,35 @@ __rte_thread_uninit(void)
>  struct rte_thread_ctrl_params {
>  	void *(*start_routine)(void *);
>  	void *arg;
> -	pthread_barrier_t configured;
> -	unsigned int refcnt;
> +	int ret;
> +	/* Synchronization variable between the control thread
> +	 * and the thread calling rte_ctrl_thread_create function.
> +	 * 0 - Initialized
> +	 * 1 - Control thread is running successfully
> +	 * 2 - Control thread encountered an error. 'ret' has the
> +	 *     error code.
> +	 */
> +	unsigned int sync;

what do you think about using an enum or defines?

>  };
>  
> -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> -{
> -	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
> -		(void)pthread_barrier_destroy(&params->configured);
> -		free(params);
> -	}
> -}
> -
>  static void *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 *);
> +	void *(*start_routine)(void *) = params->start_routine;
>  	void *routine_arg = params->arg;
>  
>  	__rte_thread_init(rte_lcore_id(), cpuset);
> -
> -	pthread_barrier_wait(&params->configured);
> -	start_routine = params->start_routine;
> -	ctrl_params_free(params);
> -
> -	if (start_routine == NULL)
> +	params->ret = pthread_setaffinity_np(pthread_self(),
> +				sizeof(*cpuset), cpuset);
> +	if (params->ret != 0) {
> +		__atomic_store_n(&params->sync, 2, __ATOMIC_RELEASE);
>  		return NULL;
> +	}

Sorry if the question is stupid (I'm still not familiar with C++11 atomic
built-ins), but do we have the assurance that params->ret is set in memory
before params->sync is set? See below at [1].

> +
> +	__atomic_store_n(&params->sync, 1, __ATOMIC_RELEASE);
>  
>  	return start_routine(routine_arg);
>  }
> @@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>  		const pthread_attr_t *attr,
>  		void *(*start_routine)(void *), 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;
>  	int ret;
>  
> @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>  
>  	params->start_routine = start_routine;
>  	params->arg = arg;
> -	params->refcnt = 2;
> -
> -	ret = pthread_barrier_init(&params->configured, NULL, 2);
> -	if (ret != 0)
> -		goto fail_no_barrier;
> +	params->ret = 0;
> +	params->sync = 0;
>  
>  	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>  	if (ret != 0)
> -		goto fail_with_barrier;
> +		goto thread_create_failed;
>  
>  	if (name != NULL) {
>  		ret = rte_thread_setname(*thread, name);
> @@ -236,24 +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>  				"Cannot set name for ctrl thread\n");
>  	}
>  
> -	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> -	if (ret != 0)
> -		params->start_routine = NULL;
> +	/* Wait for the control thread to initialize successfully */
> +	while (!__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE))
> +		rte_pause();

One difference between this implementation and the previous one is this
busy loop. rte_pause() relaxes the cpu, but will not make the calling
thread to sleep and wait for the sync event. So here we can spin a quite
long time until the other thread is scheduled by the OS.

> +	ret = params->ret;

[1]

Here, it is expected that when params->ret is seen as set before
param->sync.

>  
> -	pthread_barrier_wait(&params->configured);
> -	ctrl_params_free(params);
> +	free(params);
>  
> -	if (ret != 0)
> -		/* start_routine has been set to NULL above; */
> -		/* ctrl thread will exit immediately */
> +	if (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) != 1)

it suggest == instead of !=, like this:

  if (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) == CTRL_THREAD_ERR)


> +		/* ctrl thread is exiting */
>  		pthread_join(*thread, NULL);
>  
>  	return -ret;
>  
> -fail_with_barrier:
> -	(void)pthread_barrier_destroy(&params->configured);
> +thread_create_failed:
>  
> -fail_no_barrier:
>  	free(params);
>  
>  	return -ret;
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 1550b75da0..f1cc5e38dc 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -420,10 +420,10 @@ rte_thread_unregister(void);
>  /**
>   * Create a control thread.
>   *
> - * Wrapper to pthread_create(), pthread_setname_np() and
> - * pthread_setaffinity_np(). 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.
> + * 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.

The description is indeed better. Maybe it is the opportunity to
highlight that if the name cannot be set, no error is returned, see
commit 368a91d6bdc8 ("eal: ignore failure of naming a control thread")

>   *
>   * @param thread
>   *   Filled with the thread id of the new created thread.
> -- 
> 2.17.1
> 

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> Hi Honnappa,
> 
> On Mon, Aug 02, 2021 at 12:16:52AM -0500, Honnappa Nagarahalli wrote:
> > The current described behaviour of rte_ctrl_thread_create is rigid
> > which makes the implementation of the function complex.
> > The behavior is abstracted to allow for simplified implementation.
> 
> I agree that the behavior description should not reference the pthread
> functions, however I don't think the current description prevents from rewriting
> the code as you do.
Ok

> 
> I think it would be better to explain in commit log why the proposed code is
> simpler than the current one.
> 
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> > v2: Used compiler's C++11 atomic built-ins to access the shared variable.
> >
> >  lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
> >  lib/eal/include/rte_lcore.h        |  8 ++--
> >  2 files changed, 32 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/eal/common/eal_common_thread.c
> > b/lib/eal/common/eal_common_thread.c
> > index 1a52f42a2b..e3e0bf4bff 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -169,35 +169,35 @@ __rte_thread_uninit(void)  struct
> > rte_thread_ctrl_params {
> >  	void *(*start_routine)(void *);
> >  	void *arg;
> > -	pthread_barrier_t configured;
> > -	unsigned int refcnt;
> > +	int ret;
> > +	/* Synchronization variable between the control thread
> > +	 * and the thread calling rte_ctrl_thread_create function.
> > +	 * 0 - Initialized
> > +	 * 1 - Control thread is running successfully
> > +	 * 2 - Control thread encountered an error. 'ret' has the
> > +	 *     error code.
> > +	 */
> > +	unsigned int sync;
> 
> what do you think about using an enum or defines?
Will use defines

> 
> >  };
> >
> > -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> > -{
> > -	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) ==
> 0) {
> > -		(void)pthread_barrier_destroy(&params->configured);
> > -		free(params);
> > -	}
> > -}
> > -
> >  static void *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 *);
> > +	void *(*start_routine)(void *) = params->start_routine;
> >  	void *routine_arg = params->arg;
> >
> >  	__rte_thread_init(rte_lcore_id(), cpuset);
> > -
> > -	pthread_barrier_wait(&params->configured);
> > -	start_routine = params->start_routine;
> > -	ctrl_params_free(params);
> > -
> > -	if (start_routine == NULL)
> > +	params->ret = pthread_setaffinity_np(pthread_self(),
> > +				sizeof(*cpuset), cpuset);
> > +	if (params->ret != 0) {
> > +		__atomic_store_n(&params->sync, 2, __ATOMIC_RELEASE);
> >  		return NULL;
> > +	}
> 
> Sorry if the question is stupid (I'm still not familiar with C++11 atomic built-ins),
> but do we have the assurance that params->ret is set in memory before
> params->sync is set? See below at [1].
Yes, the '__ATOMIC_RELEASE' store ensures that all prior memory operations are completed before 'sync' is updated.

> 
> > +
> > +	__atomic_store_n(&params->sync, 1, __ATOMIC_RELEASE);
> >
> >  	return start_routine(routine_arg);
> >  }
> > @@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char
> *name,
> >  		const pthread_attr_t *attr,
> >  		void *(*start_routine)(void *), 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;
> >  	int ret;
> >
> > @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const
> > char *name,
> >
> >  	params->start_routine = start_routine;
> >  	params->arg = arg;
> > -	params->refcnt = 2;
> > -
> > -	ret = pthread_barrier_init(&params->configured, NULL, 2);
> > -	if (ret != 0)
> > -		goto fail_no_barrier;
> > +	params->ret = 0;
> > +	params->sync = 0;
> >
> >  	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> >  	if (ret != 0)
> > -		goto fail_with_barrier;
> > +		goto thread_create_failed;
> >
> >  	if (name != NULL) {
> >  		ret = rte_thread_setname(*thread, name); @@ -236,24
> +230,21 @@
> > rte_ctrl_thread_create(pthread_t *thread, const char *name,
> >  				"Cannot set name for ctrl thread\n");
> >  	}
> >
> > -	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> > -	if (ret != 0)
> > -		params->start_routine = NULL;
> > +	/* Wait for the control thread to initialize successfully */
> > +	while (!__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE))
> > +		rte_pause();
> 
> One difference between this implementation and the previous one is this busy
> loop. rte_pause() relaxes the cpu, but will not make the calling thread to sleep
> and wait for the sync event. So here we can spin a quite long time until the
> other thread is scheduled by the OS.
Yes, this is a difference. We could add a microsleep to allow for the OS to un-schedule the current thread.

> 
> > +	ret = params->ret;
> 
> [1]
> 
> Here, it is expected that when params->ret is seen as set before
> param->sync.
Yes, the __ATOMIC_ACQUIRE load ensures that the params->ret is loaded only after params->sync is loaded.

> 
> >
> > -	pthread_barrier_wait(&params->configured);
> > -	ctrl_params_free(params);
> > +	free(params);
> >
> > -	if (ret != 0)
> > -		/* start_routine has been set to NULL above; */
> > -		/* ctrl thread will exit immediately */
> > +	if (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) != 1)
> 
> it suggest == instead of !=, like this:
> 
>   if (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) ==
> CTRL_THREAD_ERR)
Ok

> 
> 
> > +		/* ctrl thread is exiting */
> >  		pthread_join(*thread, NULL);
> >
> >  	return -ret;
> >
> > -fail_with_barrier:
> > -	(void)pthread_barrier_destroy(&params->configured);
> > +thread_create_failed:
> >
> > -fail_no_barrier:
> >  	free(params);
> >
> >  	return -ret;
> > diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> > index 1550b75da0..f1cc5e38dc 100644
> > --- a/lib/eal/include/rte_lcore.h
> > +++ b/lib/eal/include/rte_lcore.h
> > @@ -420,10 +420,10 @@ rte_thread_unregister(void);
> >  /**
> >   * Create a control thread.
> >   *
> > - * Wrapper to pthread_create(), pthread_setname_np() and
> > - * pthread_setaffinity_np(). 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.
> > + * 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.
> 
> The description is indeed better. Maybe it is the opportunity to highlight that if
> the name cannot be set, no error is returned, see commit 368a91d6bdc8 ("eal:
> ignore failure of naming a control thread")
Makes sense, will add.

> 
> >   *
> >   * @param thread
> >   *   Filled with the thread id of the new created thread.
> > --
> > 2.17.1
> >

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Stephen Hemminger]

On Tue, 24 Aug 2021 20:03:03 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:

> > One difference between this implementation and the previous one is this busy
> > loop. rte_pause() relaxes the cpu, but will not make the calling thread to sleep
> > and wait for the sync event. So here we can spin a quite long time until the
> > other thread is scheduled by the OS.  
> Yes, this is a difference. We could add a microsleep to allow for the OS to un-schedule the current thread.

Why not use sched_yield() here?

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> On Tue, 24 Aug 2021 20:03:03 +0000
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> 
> > > One difference between this implementation and the previous one is
> > > this busy loop. rte_pause() relaxes the cpu, but will not make the
> > > calling thread to sleep and wait for the sync event. So here we can
> > > spin a quite long time until the other thread is scheduled by the OS.
> > Yes, this is a difference. We could add a microsleep to allow for the OS to un-
> schedule the current thread.
> 
> Why not use sched_yield() here?
Makes sense

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Mattias Rönnblom]

On 2021-08-24 23:30, Stephen Hemminger wrote:
> On Tue, 24 Aug 2021 20:03:03 +0000
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> 
>>> One difference between this implementation and the previous one is this busy
>>> loop. rte_pause() relaxes the cpu, but will not make the calling thread to sleep
>>> and wait for the sync event. So here we can spin a quite long time until the
>>> other thread is scheduled by the OS.
>> Yes, this is a difference. We could add a microsleep to allow for the OS to un-schedule the current thread.
> 
> Why not use sched_yield() here?
> 

The man page is not exactly encouraging the use sched_yield on CFS.

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> On 2021-08-24 23:30, Stephen Hemminger wrote:
> > On Tue, 24 Aug 2021 20:03:03 +0000
> > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> >
> >>> One difference between this implementation and the previous one is
> >>> this busy loop. rte_pause() relaxes the cpu, but will not make the
> >>> calling thread to sleep and wait for the sync event. So here we can
> >>> spin a quite long time until the other thread is scheduled by the OS.
> >> Yes, this is a difference. We could add a microsleep to allow for the OS to
> un-schedule the current thread.
> >
> > Why not use sched_yield() here?
> >
> 
> The man page is not exactly encouraging the use sched_yield on CFS.
Sorry, what is CFS?
There are already several uses of sched_yield in the code.

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Mattias Rönnblom]

On 2021-08-25 17:31, Honnappa Nagarahalli wrote:
> <snip>
> 
>>
>> On 2021-08-24 23:30, Stephen Hemminger wrote:
>>> On Tue, 24 Aug 2021 20:03:03 +0000
>>> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
>>>
>>>>> One difference between this implementation and the previous one is
>>>>> this busy loop. rte_pause() relaxes the cpu, but will not make the
>>>>> calling thread to sleep and wait for the sync event. So here we can
>>>>> spin a quite long time until the other thread is scheduled by the OS.
>>>> Yes, this is a difference. We could add a microsleep to allow for the OS to
>> un-schedule the current thread.
>>>
>>> Why not use sched_yield() here?
>>>
>>
>> The man page is not exactly encouraging the use sched_yield on CFS.
> Sorry, what is CFS?
> There are already several uses of sched_yield in the code.
> 

CFS is the Linux best-effort scheduler and default scheduling policy, 
and likely used by most DPDK applications.

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>
> >
> >>
> >> On 2021-08-24 23:30, Stephen Hemminger wrote:
> >>> On Tue, 24 Aug 2021 20:03:03 +0000
> >>> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> >>>
> >>>>> One difference between this implementation and the previous one is
> >>>>> this busy loop. rte_pause() relaxes the cpu, but will not make the
> >>>>> calling thread to sleep and wait for the sync event. So here we
> >>>>> can spin a quite long time until the other thread is scheduled by the OS.
> >>>> Yes, this is a difference. We could add a microsleep to allow for
> >>>> the OS to
> >> un-schedule the current thread.
> >>>
> >>> Why not use sched_yield() here?
> >>>
> >>
> >> The man page is not exactly encouraging the use sched_yield on CFS.
> > Sorry, what is CFS?
> > There are already several uses of sched_yield in the code.
> >
> 
> CFS is the Linux best-effort scheduler and default scheduling policy, and likely
> used by most DPDK applications.
Thanks, I read some of the issues. If sched_yield has to be used, the API needs to be implemented for Linux/Windows/FreeBSD. From my perspective, this API is called mostly during init time. I do not think it is worth duplicating the code and maintaining it. I would prefer to go with usleep.

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>
> 
> On Tue, 24 Aug 2021 20:03:03 +0000
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> 
> > > One difference between this implementation and the previous one is
> > > this busy loop. rte_pause() relaxes the cpu, but will not make the
> > > calling thread to sleep and wait for the sync event. So here we can
> > > spin a quite long time until the other thread is scheduled by the OS.
> > Yes, this is a difference. We could add a microsleep to allow for the OS to un-
> schedule the current thread.
> 
> Why not use sched_yield() here?
This means, it is not portable to Windows. The function needs to be moved to OS specific files. Are there any guidelines on creating OS specific functions? Can I create in this case?

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Mattias Rönnblom]

On 2021-08-02 07:16, Honnappa Nagarahalli wrote:
> The current described behaviour of rte_ctrl_thread_create is
> rigid which makes the implementation of the function complex.
> The behavior is abstracted to allow for simplified implementation.
> 

Have you considered using a POSIX condition variable instead of atomics 
for synchronization?

> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> v2: Used compiler's C++11 atomic built-ins to access the shared variable.
> 
>   lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
>   lib/eal/include/rte_lcore.h        |  8 ++--
>   2 files changed, 32 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 1a52f42a2b..e3e0bf4bff 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -169,35 +169,35 @@ __rte_thread_uninit(void)
>   struct rte_thread_ctrl_params {
>   	void *(*start_routine)(void *);
>   	void *arg;
> -	pthread_barrier_t configured;
> -	unsigned int refcnt;
> +	int ret;
> +	/* Synchronization variable between the control thread
> +	 * and the thread calling rte_ctrl_thread_create function.
> +	 * 0 - Initialized
> +	 * 1 - Control thread is running successfully
> +	 * 2 - Control thread encountered an error. 'ret' has the
> +	 *     error code.
> +	 */
> +	unsigned int sync;
>   };
>   
> -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> -{
> -	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
> -		(void)pthread_barrier_destroy(&params->configured);
> -		free(params);
> -	}
> -}
> -
>   static void *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 *);
> +	void *(*start_routine)(void *) = params->start_routine;
>   	void *routine_arg = params->arg;
>   
>   	__rte_thread_init(rte_lcore_id(), cpuset);
> -
> -	pthread_barrier_wait(&params->configured);
> -	start_routine = params->start_routine;
> -	ctrl_params_free(params);
> -
> -	if (start_routine == NULL)
> +	params->ret = pthread_setaffinity_np(pthread_self(),
> +				sizeof(*cpuset), cpuset);
> +	if (params->ret != 0) {
> +		__atomic_store_n(&params->sync, 2, __ATOMIC_RELEASE);
>   		return NULL;
> +	}
> +
> +	__atomic_store_n(&params->sync, 1, __ATOMIC_RELEASE);
>   
>   	return start_routine(routine_arg);
>   }
> @@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>   		const pthread_attr_t *attr,
>   		void *(*start_routine)(void *), 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;
>   	int ret;
>   
> @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>   
>   	params->start_routine = start_routine;
>   	params->arg = arg;
> -	params->refcnt = 2;
> -
> -	ret = pthread_barrier_init(&params->configured, NULL, 2);
> -	if (ret != 0)
> -		goto fail_no_barrier;
> +	params->ret = 0;
> +	params->sync = 0;
>   
>   	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>   	if (ret != 0)
> -		goto fail_with_barrier;
> +		goto thread_create_failed;
>   
>   	if (name != NULL) {
>   		ret = rte_thread_setname(*thread, name);
> @@ -236,24 +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>   				"Cannot set name for ctrl thread\n");
>   	}
>   
> -	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> -	if (ret != 0)
> -		params->start_routine = NULL;
> +	/* Wait for the control thread to initialize successfully */
> +	while (!__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE))
> +		rte_pause();
> +	ret = params->ret;
>   
> -	pthread_barrier_wait(&params->configured);
> -	ctrl_params_free(params);
> +	free(params);
>   
> -	if (ret != 0)
> -		/* start_routine has been set to NULL above; */
> -		/* ctrl thread will exit immediately */
> +	if (__atomic_load_n(&params->sync, __ATOMIC_ACQUIRE) != 1)
> +		/* ctrl thread is exiting */
>   		pthread_join(*thread, NULL);
>   
>   	return -ret;
>   
> -fail_with_barrier:
> -	(void)pthread_barrier_destroy(&params->configured);
> +thread_create_failed:
>   
> -fail_no_barrier:
>   	free(params);
>   
>   	return -ret;
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 1550b75da0..f1cc5e38dc 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -420,10 +420,10 @@ rte_thread_unregister(void);
>   /**
>    * Create a control thread.
>    *
> - * Wrapper to pthread_create(), pthread_setname_np() and
> - * pthread_setaffinity_np(). 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.
> + * 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.
>    *
>    * @param thread
>    *   Filled with the thread id of the new created thread.
> 

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> On 2021-08-02 07:16, Honnappa Nagarahalli wrote:
> > The current described behaviour of rte_ctrl_thread_create is rigid
> > which makes the implementation of the function complex.
> > The behavior is abstracted to allow for simplified implementation.
> >
> 
> Have you considered using a POSIX condition variable instead of atomics for
> synchronization?
No, I have not considered. The current implementation is complex because of the error handling of pthread_barrier_xxx APIs. I am thinking similar complexity will come in with the condition variable functions.

I am using atomic variable in this patch to synchronize which does not need much error handling which simplifies the implementation.

<snip>

Re: [dpdk-dev] [RFC v2] eal: simplify the implementation of rte_ctrl_thread_create

[Stephen Hemminger]

On Wed, 25 Aug 2021 15:23:08 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:

> <snip>
> 
> > 
> > On 2021-08-02 07:16, Honnappa Nagarahalli wrote:  
> > > The current described behaviour of rte_ctrl_thread_create is rigid
> > > which makes the implementation of the function complex.
> > > The behavior is abstracted to allow for simplified implementation.
> > >  
> > 
> > Have you considered using a POSIX condition variable instead of atomics for
> > synchronization?  
> No, I have not considered. The current implementation is complex because of the error handling of pthread_barrier_xxx APIs. I am thinking similar complexity will come in with the condition variable functions.
> 
> I am using atomic variable in this patch to synchronize which does not need much error handling which simplifies the implementation.
> 
> <snip>

The problem with condition variables is that it then adds a mutex
as well.

Re: [dpdk-dev] [RFC] eal: simplify the implementation of rte_ctrl_thread_create

[Luc Pelletier]

Hi Honnappa,

(Sorry if you get this message twice, I forgot to reply all the first time)

Sorry for the late reply. I was also away.

I have only made one small contribution to DPDK so I'll defer to
others to decide whether this patch should be accepted. When I
submitted my patch, I got the feeling that busy loops were somewhat
frowned upon but it might be a good solution here.

Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> a écrit :
<snip>
> +       /* Synchronization variable between the control thread
> +        * and the thread calling rte_ctrl_thread_create function.
> +        * 0 - Initialized
> +        * 1 - Control thread is running successfully
> +        * 2 - Control thread encountered an error. 'ret' has the
> +        *     error code.
> +        */
> +       unsigned int sync;

This is highly subjective but the name of this variable doesn't
clearly indicate that once it is set, the control thread is
effectively done with the rte_thread_ctrl_params. The downside that I
can see (as opposed to the previous reference counting approach) is
that anyone making further changes would have to be cognizant of that
fact. Otherwise, any new code that accesses "sync" in the control
thread, after "sync" has been set, would create a race.

<snip>
>         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>         if (ret != 0)
> -               goto fail_with_barrier;
> +               goto thread_create_failed;

This is also highly subjective but since the goto label is only used
here, you could completely get rid of the goto and simply free and
return here.

<snip>
> +       /* Wait for the control thread to initialize successfully */
> +       while (!params->sync)
> +               rte_pause();
> +       ret = params->ret;
>
> -       pthread_barrier_wait(&params->configured);
> -       ctrl_params_free(params);
> +       free(params);

Maybe I'm missing something but it seems like you're accessing
params->sync after this line (use after free). I assume you meant to
add this line after the subsequent if block?

Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> a écrit :
>
> The current described behaviour of rte_ctrl_thread_create is
> rigid which makes the implementation of the function complex.
> The behavior is abstracted to allow for simplified implementation.
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
>  lib/eal/include/rte_lcore.h        |  8 ++--
>  2 files changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 1a52f42a2b..86cacd840c 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -169,35 +169,35 @@ __rte_thread_uninit(void)
>  struct rte_thread_ctrl_params {
>         void *(*start_routine)(void *);
>         void *arg;
> -       pthread_barrier_t configured;
> -       unsigned int refcnt;
> +       int ret;
> +       /* Synchronization variable between the control thread
> +        * and the thread calling rte_ctrl_thread_create function.
> +        * 0 - Initialized
> +        * 1 - Control thread is running successfully
> +        * 2 - Control thread encountered an error. 'ret' has the
> +        *     error code.
> +        */
> +       unsigned int sync;
>  };
>
> -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> -{
> -       if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
> -               (void)pthread_barrier_destroy(&params->configured);
> -               free(params);
> -       }
> -}
> -
>  static void *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 *);
> +       void *(*start_routine)(void *) = params->start_routine;
>         void *routine_arg = params->arg;
>
>         __rte_thread_init(rte_lcore_id(), cpuset);
> -
> -       pthread_barrier_wait(&params->configured);
> -       start_routine = params->start_routine;
> -       ctrl_params_free(params);
> -
> -       if (start_routine == NULL)
> +       params->ret = pthread_setaffinity_np(pthread_self(),
> +                               sizeof(*cpuset), cpuset);
> +       if (params->ret != 0) {
> +               params->sync = 2;
>                 return NULL;
> +       }
> +
> +       params->sync = 1;
>
>         return start_routine(routine_arg);
>  }
> @@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>                 const pthread_attr_t *attr,
>                 void *(*start_routine)(void *), 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;
>         int ret;
>
> @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>
>         params->start_routine = start_routine;
>         params->arg = arg;
> -       params->refcnt = 2;
> -
> -       ret = pthread_barrier_init(&params->configured, NULL, 2);
> -       if (ret != 0)
> -               goto fail_no_barrier;
> +       params->ret = 0;
> +       params->sync = 0;
>
>         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>         if (ret != 0)
> -               goto fail_with_barrier;
> +               goto thread_create_failed;
>
>         if (name != NULL) {
>                 ret = rte_thread_setname(*thread, name);
> @@ -236,24 +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>                                 "Cannot set name for ctrl thread\n");
>         }
>
> -       ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> -       if (ret != 0)
> -               params->start_routine = NULL;
> +       /* Wait for the control thread to initialize successfully */
> +       while (!params->sync)
> +               rte_pause();
> +       ret = params->ret;
>
> -       pthread_barrier_wait(&params->configured);
> -       ctrl_params_free(params);
> +       free(params);
>
> -       if (ret != 0)
> -               /* start_routine has been set to NULL above; */
> -               /* ctrl thread will exit immediately */
> +       if (params->sync != 1)
> +               /* ctrl thread is exiting */
>                 pthread_join(*thread, NULL);
>
>         return -ret;
>
> -fail_with_barrier:
> -       (void)pthread_barrier_destroy(&params->configured);
> +thread_create_failed:
>
> -fail_no_barrier:
>         free(params);
>
>         return -ret;
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 1550b75da0..f1cc5e38dc 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -420,10 +420,10 @@ rte_thread_unregister(void);
>  /**
>   * Create a control thread.
>   *
> - * Wrapper to pthread_create(), pthread_setname_np() and
> - * pthread_setaffinity_np(). 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.
> + * 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.
>   *
>   * @param thread
>   *   Filled with the thread id of the new created thread.
> --
> 2.17.1
>

Re: [dpdk-dev] [RFC] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> Hi Honnappa,
> 
> (Sorry if you get this message twice, I forgot to reply all the first time)
> 
> Sorry for the late reply. I was also away.
No problem, thanks for your comments.

> 
> I have only made one small contribution to DPDK so I'll defer to others to
> decide whether this patch should be accepted. When I submitted my patch, I
> got the feeling that busy loops were somewhat frowned upon but it might be
> a good solution here.
IMO, it should be ok here since the busy loop is for a small period. Adding sched_yield should help reduce the actual spinning.

> 
> Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
> <honnappa.nagarahalli@arm.com> a écrit :
> <snip>
> > +       /* Synchronization variable between the control thread
> > +        * and the thread calling rte_ctrl_thread_create function.
> > +        * 0 - Initialized
> > +        * 1 - Control thread is running successfully
> > +        * 2 - Control thread encountered an error. 'ret' has the
> > +        *     error code.
> > +        */
> > +       unsigned int sync;
> 
> This is highly subjective but the name of this variable doesn't clearly indicate
> that once it is set, the control thread is effectively done with the
> rte_thread_ctrl_params. The downside that I can see (as opposed to the
> previous reference counting approach) is that anyone making further changes
> would have to be cognizant of that fact. Otherwise, any new code that
> accesses "sync" in the control thread, after "sync" has been set, would create
> a race.
May be "ctrl_thread_status" would be better.

> 
> <snip>
> >         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> >         if (ret != 0)
> > -               goto fail_with_barrier;
> > +               goto thread_create_failed;
> 
> This is also highly subjective but since the goto label is only used here, you
> could completely get rid of the goto and simply free and return here.
Thanks, agree.

> 
> <snip>
> > +       /* Wait for the control thread to initialize successfully */
> > +       while (!params->sync)
> > +               rte_pause();
> > +       ret = params->ret;
> >
> > -       pthread_barrier_wait(&params->configured);
> > -       ctrl_params_free(params);
> > +       free(params);
> 
> Maybe I'm missing something but it seems like you're accessing
> params->sync after this line (use after free). I assume you meant to
> add this line after the subsequent if block?
You are correct. I realized this yesterday while incorporating review comments. That made me think, we need a test case. I am adding a test case as well.

> 
> Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
> <honnappa.nagarahalli@arm.com> a écrit :
> >
> > The current described behaviour of rte_ctrl_thread_create is rigid
> > which makes the implementation of the function complex.
> > The behavior is abstracted to allow for simplified implementation.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> >  lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
> >  lib/eal/include/rte_lcore.h        |  8 ++--
> >  2 files changed, 32 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/eal/common/eal_common_thread.c
> > b/lib/eal/common/eal_common_thread.c
> > index 1a52f42a2b..86cacd840c 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -169,35 +169,35 @@ __rte_thread_uninit(void)  struct
> > rte_thread_ctrl_params {
> >         void *(*start_routine)(void *);
> >         void *arg;
> > -       pthread_barrier_t configured;
> > -       unsigned int refcnt;
> > +       int ret;
> > +       /* Synchronization variable between the control thread
> > +        * and the thread calling rte_ctrl_thread_create function.
> > +        * 0 - Initialized
> > +        * 1 - Control thread is running successfully
> > +        * 2 - Control thread encountered an error. 'ret' has the
> > +        *     error code.
> > +        */
> > +       unsigned int sync;
> >  };
> >
> > -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> > -{
> > -       if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) ==
> 0) {
> > -               (void)pthread_barrier_destroy(&params->configured);
> > -               free(params);
> > -       }
> > -}
> > -
> >  static void *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 *);
> > +       void *(*start_routine)(void *) = params->start_routine;
> >         void *routine_arg = params->arg;
> >
> >         __rte_thread_init(rte_lcore_id(), cpuset);
> > -
> > -       pthread_barrier_wait(&params->configured);
> > -       start_routine = params->start_routine;
> > -       ctrl_params_free(params);
> > -
> > -       if (start_routine == NULL)
> > +       params->ret = pthread_setaffinity_np(pthread_self(),
> > +                               sizeof(*cpuset), cpuset);
> > +       if (params->ret != 0) {
> > +               params->sync = 2;
> >                 return NULL;
> > +       }
> > +
> > +       params->sync = 1;
> >
> >         return start_routine(routine_arg);  } @@ -207,9 +207,6 @@
> > rte_ctrl_thread_create(pthread_t *thread, const char *name,
> >                 const pthread_attr_t *attr,
> >                 void *(*start_routine)(void *), 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;
> >         int ret;
> >
> > @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const
> > char *name,
> >
> >         params->start_routine = start_routine;
> >         params->arg = arg;
> > -       params->refcnt = 2;
> > -
> > -       ret = pthread_barrier_init(&params->configured, NULL, 2);
> > -       if (ret != 0)
> > -               goto fail_no_barrier;
> > +       params->ret = 0;
> > +       params->sync = 0;
> >
> >         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> >         if (ret != 0)
> > -               goto fail_with_barrier;
> > +               goto thread_create_failed;
> >
> >         if (name != NULL) {
> >                 ret = rte_thread_setname(*thread, name); @@ -236,24
> > +230,21 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
> >                                 "Cannot set name for ctrl thread\n");
> >         }
> >
> > -       ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> > -       if (ret != 0)
> > -               params->start_routine = NULL;
> > +       /* Wait for the control thread to initialize successfully */
> > +       while (!params->sync)
> > +               rte_pause();
> > +       ret = params->ret;
> >
> > -       pthread_barrier_wait(&params->configured);
> > -       ctrl_params_free(params);
> > +       free(params);
> >
> > -       if (ret != 0)
> > -               /* start_routine has been set to NULL above; */
> > -               /* ctrl thread will exit immediately */
> > +       if (params->sync != 1)
> > +               /* ctrl thread is exiting */
> >                 pthread_join(*thread, NULL);
> >
> >         return -ret;
> >
> > -fail_with_barrier:
> > -       (void)pthread_barrier_destroy(&params->configured);
> > +thread_create_failed:
> >
> > -fail_no_barrier:
> >         free(params);
> >
> >         return -ret;
> > diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> > index 1550b75da0..f1cc5e38dc 100644
> > --- a/lib/eal/include/rte_lcore.h
> > +++ b/lib/eal/include/rte_lcore.h
> > @@ -420,10 +420,10 @@ rte_thread_unregister(void);
> >  /**
> >   * Create a control thread.
> >   *
> > - * Wrapper to pthread_create(), pthread_setname_np() and
> > - * pthread_setaffinity_np(). 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.
> > + * 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.
> >   *
> >   * @param thread
> >   *   Filled with the thread id of the new created thread.
> > --
> > 2.17.1
> >

[dpdk-dev] [PATCH v3 1/2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

Remove the usage of pthread barrier and replace it with
synchronization using atomic variable. This also removes
the use of reference count required to synchronize freeing
the memory.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
v3:
1) RFC to Patch
2) Changed commit log and API description [Olivier]
3) Used sched_yield while spinning [Stephen]
4) Fixed use after free
5) Added a test case

v2:
1) Used compiler's C++11 atomic built-ins to access the shared variable.

 lib/eal/common/eal_common_thread.c | 83 +++++++++++++++---------------
 lib/eal/include/rte_lcore.h        |  9 ++--
 2 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 1a52f42a2b..b0af965b7b 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -166,38 +166,46 @@ __rte_thread_uninit(void)
 	RTE_PER_LCORE(_lcore_id) = LCORE_ID_ANY;
 }
 
+#define _RTE_CTRL_THREAD_LAUNCHING 0
+#define _RTE_CTRL_THREAD_RUNNING 1
+#define _RTE_CTRL_THREAD_ERROR 2
+
 struct rte_thread_ctrl_params {
 	void *(*start_routine)(void *);
 	void *arg;
-	pthread_barrier_t configured;
-	unsigned int refcnt;
+	int ret;
+	/* Control thread status.
+	 *
+	 * _RTE_CTRL_THREAD_LAUNCHING - Yet to call pthread_create function
+	 * _RTE_CTRL_THREAD_RUNNING - Control thread is running successfully
+	 * _RTE_CTRL_THREAD_ERROR - Control thread encountered an error.
+	 *                          'ret' has the error code.
+	 */
+	unsigned int ctrl_thread_status;
 };
 
-static void ctrl_params_free(struct rte_thread_ctrl_params *params)
-{
-	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
-		(void)pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
-}
-
 static void *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 *);
+	void *(*start_routine)(void *) = params->start_routine;
 	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
-
-	pthread_barrier_wait(&params->configured);
-	start_routine = params->start_routine;
-	ctrl_params_free(params);
-
-	if (start_routine == NULL)
+	params->ret = pthread_setaffinity_np(pthread_self(),
+				sizeof(*cpuset), cpuset);
+	if (params->ret != 0) {
+		__atomic_store_n(&params->ctrl_thread_status,
+					_RTE_CTRL_THREAD_ERROR,
+					__ATOMIC_RELEASE);
 		return NULL;
+	}
+
+	__atomic_store_n(&params->ctrl_thread_status,
+				_RTE_CTRL_THREAD_RUNNING,
+				__ATOMIC_RELEASE);
 
 	return start_routine(routine_arg);
 }
@@ -207,10 +215,8 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 		const pthread_attr_t *attr,
 		void *(*start_routine)(void *), 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;
+	unsigned int ctrl_thread_status;
 	int ret;
 
 	params = malloc(sizeof(*params));
@@ -219,15 +225,14 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params->start_routine = start_routine;
 	params->arg = arg;
-	params->refcnt = 2;
-
-	ret = pthread_barrier_init(&params->configured, NULL, 2);
-	if (ret != 0)
-		goto fail_no_barrier;
+	params->ret = 0;
+	params->ctrl_thread_status = _RTE_CTRL_THREAD_LAUNCHING;
 
 	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
-	if (ret != 0)
-		goto fail_with_barrier;
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
 
 	if (name != NULL) {
 		ret = rte_thread_setname(*thread, name);
@@ -236,24 +241,18 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 				"Cannot set name for ctrl thread\n");
 	}
 
-	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
-	if (ret != 0)
-		params->start_routine = NULL;
-
-	pthread_barrier_wait(&params->configured);
-	ctrl_params_free(params);
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+		__atomic_load_n(&params->ctrl_thread_status, __ATOMIC_ACQUIRE))
+		== _RTE_CTRL_THREAD_LAUNCHING)
+		sched_yield();
 
-	if (ret != 0)
-		/* start_routine has been set to NULL above; */
-		/* ctrl thread will exit immediately */
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == _RTE_CTRL_THREAD_ERROR)
+		/* ctrl thread is exiting */
 		pthread_join(*thread, NULL);
 
-	return -ret;
-
-fail_with_barrier:
-	(void)pthread_barrier_destroy(&params->configured);
-
-fail_no_barrier:
+	ret = params->ret;
 	free(params);
 
 	return -ret;
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 1550b75da0..b81c8b2685 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -420,10 +420,11 @@ rte_thread_unregister(void);
 /**
  * Create a control thread.
  *
- * Wrapper to pthread_create(), pthread_setname_np() and
- * pthread_setaffinity_np(). 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.
+ * 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.
-- 
2.25.1

[dpdk-dev] [PATCH v3 2/2] test/eal: add a test for rte_ctrl_thread_create

[Honnappa Nagarahalli]

Add a testcase to test launching of control threads.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 app/test/test_lcores.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index 19a7ab9fce..738148f339 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -340,6 +340,44 @@ test_non_eal_lcores_callback(unsigned int eal_threads_count)
 	return -1;
 }
 
+static void *ctrl_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = DONE;
+
+	return NULL;
+}
+
+static int
+test_crtl_thread(void)
+{
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = INIT;
+	if (rte_ctrl_thread_create(&t->id, "test_ctrl_threads",
+					NULL, ctrl_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.
+	 */
+	pthread_join(t->id, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != DONE)
+		return -1;
+
+	return 0;
+}
+
 static int
 test_lcores(void)
 {
@@ -367,6 +405,9 @@ test_lcores(void)
 	if (test_non_eal_lcores_callback(eal_threads_count) < 0)
 		return TEST_FAILED;
 
+	if (test_crtl_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
-- 
2.25.1

Re: [dpdk-dev] [PATCH v3 2/2] test/eal: add a test for rte_ctrl_thread_create

[Stephen Hemminger]

On Mon, 30 Aug 2021 11:34:13 -0500
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:

> +static int
> +test_crtl_thread(void)

Is this a typo? Did you mean test_ctrl_thread?

Re: [dpdk-dev] [PATCH v3 2/2] test/eal: add a test for rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> On Mon, 30 Aug 2021 11:34:13 -0500
> Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:
> 
> > +static int
> > +test_crtl_thread(void)
> 
> Is this a typo? Did you mean test_ctrl_thread?
Yes, thanks

Re: [dpdk-dev] [PATCH v3 1/2] eal: simplify the implementation of rte_ctrl_thread_create

[Stephen Hemminger]

On Mon, 30 Aug 2021 11:34:12 -0500
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:

> +	/* Control thread status.
> +	 *
> +	 * _RTE_CTRL_THREAD_LAUNCHING - Yet to call pthread_create function
> +	 * _RTE_CTRL_THREAD_RUNNING - Control thread is running successfully
> +	 * _RTE_CTRL_THREAD_ERROR - Control thread encountered an error.
> +	 *                          'ret' has the error code.
> +	 */
> +	unsigned int ctrl_thread_status;

An enum would be clearer here.

Re: [dpdk-dev] [PATCH v3 1/2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> On Mon, 30 Aug 2021 11:34:12 -0500
> Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:
> 
> > +	/* Control thread status.
> > +	 *
> > +	 * _RTE_CTRL_THREAD_LAUNCHING - Yet to call pthread_create
> function
> > +	 * _RTE_CTRL_THREAD_RUNNING - Control thread is running
> successfully
> > +	 * _RTE_CTRL_THREAD_ERROR - Control thread encountered an error.
> > +	 *                          'ret' has the error code.
> > +	 */
> > +	unsigned int ctrl_thread_status;
> 
> An enum would be clearer here.
I used #defines as the values are assigned to an atomic variable. We can assume that enum will be an int.

[dpdk-dev] [PATCH v4 1/2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

Remove the usage of pthread barrier and replace it with
synchronization using atomic variable. This also removes
the use of reference count required to synchronize freeing
the memory.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
v4:
1) Used rte_delay_us_sleep instead of sched_yield. Using sched_yield
   requires to maintain a Windows version of the rte_ctrl_thread_create
   API.

v3:
1) RFC to Patch
2) Changed commit log and API description [Olivier]
3) Used sched_yield while spinning [Stephen]
4) Fixed use after free
5) Added a test case

v2:
1) Used compiler's C++11 atomic built-ins to access the shared variable.

 lib/eal/common/eal_common_thread.c | 87 +++++++++++++++---------------
 lib/eal/include/rte_lcore.h        |  9 ++--
 2 files changed, 49 insertions(+), 47 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 1a52f42a2b..e8e4149f69 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -166,38 +166,44 @@ __rte_thread_uninit(void)
 	RTE_PER_LCORE(_lcore_id) = LCORE_ID_ANY;
 }
 
+enum __rte_ctrl_thread_status {
+	__RTE_CTRL_THREAD_LAUNCHING, /* Yet to call pthread_create function */
+	__RTE_CTRL_THREAD_RUNNING, /* Control thread is running successfully */
+	__RTE_CTRL_THREAD_ERROR /* Control thread encountered an error */
+};
+
 struct rte_thread_ctrl_params {
 	void *(*start_routine)(void *);
 	void *arg;
-	pthread_barrier_t configured;
-	unsigned int refcnt;
+	int ret;
+	/* Control thread status. If the status is __RTE_CTRL_THREAD_ERROR
+	 * 'ret' has the error code.
+	 */
+	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void ctrl_params_free(struct rte_thread_ctrl_params *params)
-{
-	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
-		(void)pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
-}
-
 static void *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 *);
+	void *(*start_routine)(void *) = params->start_routine;
 	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
-
-	pthread_barrier_wait(&params->configured);
-	start_routine = params->start_routine;
-	ctrl_params_free(params);
-
-	if (start_routine == NULL)
+	params->ret = pthread_setaffinity_np(pthread_self(),
+				sizeof(*cpuset), cpuset);
+	if (params->ret != 0) {
+		__atomic_store_n(&params->ctrl_thread_status,
+					__RTE_CTRL_THREAD_ERROR,
+					__ATOMIC_RELEASE);
 		return NULL;
+	}
+
+	__atomic_store_n(&params->ctrl_thread_status,
+				__RTE_CTRL_THREAD_RUNNING,
+				__ATOMIC_RELEASE);
 
 	return start_routine(routine_arg);
 }
@@ -207,10 +213,8 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 		const pthread_attr_t *attr,
 		void *(*start_routine)(void *), 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;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
 	int ret;
 
 	params = malloc(sizeof(*params));
@@ -219,15 +223,14 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params->start_routine = start_routine;
 	params->arg = arg;
-	params->refcnt = 2;
-
-	ret = pthread_barrier_init(&params->configured, NULL, 2);
-	if (ret != 0)
-		goto fail_no_barrier;
+	params->ret = 0;
+	params->ctrl_thread_status = __RTE_CTRL_THREAD_LAUNCHING;
 
 	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
-	if (ret != 0)
-		goto fail_with_barrier;
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
 
 	if (name != NULL) {
 		ret = rte_thread_setname(*thread, name);
@@ -236,24 +239,22 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 				"Cannot set name for ctrl thread\n");
 	}
 
-	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
-	if (ret != 0)
-		params->start_routine = NULL;
-
-	pthread_barrier_wait(&params->configured);
-	ctrl_params_free(params);
-
-	if (ret != 0)
-		/* start_routine has been set to NULL above; */
-		/* ctrl thread will exit immediately */
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+		__atomic_load_n(&params->ctrl_thread_status, __ATOMIC_ACQUIRE))
+		== __RTE_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 == __RTE_CTRL_THREAD_ERROR)
+		/* ctrl thread is exiting */
 		pthread_join(*thread, NULL);
 
-	return -ret;
-
-fail_with_barrier:
-	(void)pthread_barrier_destroy(&params->configured);
-
-fail_no_barrier:
+	ret = params->ret;
 	free(params);
 
 	return -ret;
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 1550b75da0..b81c8b2685 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -420,10 +420,11 @@ rte_thread_unregister(void);
 /**
  * Create a control thread.
  *
- * Wrapper to pthread_create(), pthread_setname_np() and
- * pthread_setaffinity_np(). 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.
+ * 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.
-- 
2.25.1

[dpdk-dev] [PATCH v4 2/2] test/eal: add a test for rte_ctrl_thread_create

[Honnappa Nagarahalli]

Add a testcase to test launching of control threads.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 app/test/test_lcores.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index 19a7ab9fce..35c47d5372 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -340,6 +340,44 @@ test_non_eal_lcores_callback(unsigned int eal_threads_count)
 	return -1;
 }
 
+static void *ctrl_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = DONE;
+
+	return NULL;
+}
+
+static int
+test_ctrl_thread(void)
+{
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = INIT;
+	if (rte_ctrl_thread_create(&t->id, "test_ctrl_threads",
+					NULL, ctrl_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.
+	 */
+	pthread_join(t->id, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != DONE)
+		return -1;
+
+	return 0;
+}
+
 static int
 test_lcores(void)
 {
@@ -367,6 +405,9 @@ test_lcores(void)
 	if (test_non_eal_lcores_callback(eal_threads_count) < 0)
 		return TEST_FAILED;
 
+	if (test_ctrl_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
-- 
2.25.1

Re: [dpdk-dev] [PATCH v4 1/2] eal: simplify the implementation of rte_ctrl_thread_create

[Olivier Matz]

Hi Honnappa,

On Wed, Oct 13, 2021 at 03:18:10PM -0500, Honnappa Nagarahalli wrote:
> Remove the usage of pthread barrier and replace it with
> synchronization using atomic variable. This also removes
> the use of reference count required to synchronize freeing
> the memory.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Few cosmetics comments below.

(...)

> +enum __rte_ctrl_thread_status {
> +	__RTE_CTRL_THREAD_LAUNCHING, /* Yet to call pthread_create function */
> +	__RTE_CTRL_THREAD_RUNNING, /* Control thread is running successfully */
> +	__RTE_CTRL_THREAD_ERROR /* Control thread encountered an error */
> +};
> +

Are there double underscores needed?
I think even the rte_ prefix could be removed since this is not exposed.

(...)

> @@ -236,24 +239,22 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>  				"Cannot set name for ctrl thread\n");
>  	}
>  
> -	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> -	if (ret != 0)
> -		params->start_routine = NULL;
> -
> -	pthread_barrier_wait(&params->configured);
> -	ctrl_params_free(params);
> -
> -	if (ret != 0)
> -		/* start_routine has been set to NULL above; */
> -		/* ctrl thread will exit immediately */
> +	/* Wait for the control thread to initialize successfully */
> +	while ((ctrl_thread_status =
> +		__atomic_load_n(&params->ctrl_thread_status, __ATOMIC_ACQUIRE))
> +		== __RTE_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 == __RTE_CTRL_THREAD_ERROR)
> +		/* ctrl thread is exiting */
>  		pthread_join(*thread, NULL);

While not required, I suggest to use curly brackets when the number of lines
in the body of the loop or test (including comments).


Thanks
Olivier

Re: [dpdk-dev] [PATCH v4 1/2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> 
> Hi Honnappa,
> 
> On Wed, Oct 13, 2021 at 03:18:10PM -0500, Honnappa Nagarahalli wrote:
> > Remove the usage of pthread barrier and replace it with
> > synchronization using atomic variable. This also removes the use of
> > reference count required to synchronize freeing the memory.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> 
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Few cosmetics comments below.
> 
> (...)
> 
> > +enum __rte_ctrl_thread_status {
> > +	__RTE_CTRL_THREAD_LAUNCHING, /* Yet to call pthread_create
> function */
> > +	__RTE_CTRL_THREAD_RUNNING, /* Control thread is running
> successfully */
> > +	__RTE_CTRL_THREAD_ERROR /* Control thread encountered an error
> */ };
> > +
> 
> Are there double underscores needed?
> I think even the rte_ prefix could be removed since this is not exposed.
I am fine to change this, just want to be consistent with the rest of the code. Looks like a lot of code does not use __RTE.

> 
> (...)
> 
> > @@ -236,24 +239,22 @@ rte_ctrl_thread_create(pthread_t *thread, const
> char *name,
> >  				"Cannot set name for ctrl thread\n");
> >  	}
> >
> > -	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> > -	if (ret != 0)
> > -		params->start_routine = NULL;
> > -
> > -	pthread_barrier_wait(&params->configured);
> > -	ctrl_params_free(params);
> > -
> > -	if (ret != 0)
> > -		/* start_routine has been set to NULL above; */
> > -		/* ctrl thread will exit immediately */
> > +	/* Wait for the control thread to initialize successfully */
> > +	while ((ctrl_thread_status =
> > +		__atomic_load_n(&params->ctrl_thread_status,
> __ATOMIC_ACQUIRE))
> > +		== __RTE_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 == __RTE_CTRL_THREAD_ERROR)
> > +		/* ctrl thread is exiting */
> >  		pthread_join(*thread, NULL);
> 
> While not required, I suggest to use curly brackets when the number of lines in
> the body of the loop or test (including comments).
ok
> 
> 
> Thanks
> Olivier

[dpdk-dev] [PATCH v5 1/2] eal: simplify the implementation of rte_ctrl_thread_create

[Honnappa Nagarahalli]

Remove the usage of pthread barrier and replace it with
synchronization using atomic variable. This also removes
the use of reference count required to synchronize freeing
the memory.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/eal/common/eal_common_thread.c | 87 +++++++++++++++---------------
 lib/eal/include/rte_lcore.h        |  9 ++--
 2 files changed, 50 insertions(+), 46 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 1a52f42a2b..c602bd2a0d 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -166,38 +166,44 @@ __rte_thread_uninit(void)
 	RTE_PER_LCORE(_lcore_id) = LCORE_ID_ANY;
 }
 
+enum __rte_ctrl_thread_status {
+	CTRL_THREAD_LAUNCHING, /* Yet to call pthread_create function */
+	CTRL_THREAD_RUNNING, /* Control thread is running successfully */
+	CTRL_THREAD_ERROR /* Control thread encountered an error */
+};
+
 struct rte_thread_ctrl_params {
 	void *(*start_routine)(void *);
 	void *arg;
-	pthread_barrier_t configured;
-	unsigned int refcnt;
+	int ret;
+	/* Control thread status. If the status is CTRL_THREAD_ERROR
+	 * 'ret' has the error code.
+	 */
+	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void ctrl_params_free(struct rte_thread_ctrl_params *params)
-{
-	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
-		(void)pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
-}
-
 static void *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 *);
+	void *(*start_routine)(void *) = params->start_routine;
 	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
-
-	pthread_barrier_wait(&params->configured);
-	start_routine = params->start_routine;
-	ctrl_params_free(params);
-
-	if (start_routine == NULL)
+	params->ret = pthread_setaffinity_np(pthread_self(),
+				sizeof(*cpuset), cpuset);
+	if (params->ret != 0) {
+		__atomic_store_n(&params->ctrl_thread_status,
+					CTRL_THREAD_ERROR,
+					__ATOMIC_RELEASE);
 		return NULL;
+	}
+
+	__atomic_store_n(&params->ctrl_thread_status,
+				CTRL_THREAD_RUNNING,
+				__ATOMIC_RELEASE);
 
 	return start_routine(routine_arg);
 }
@@ -207,10 +213,8 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 		const pthread_attr_t *attr,
 		void *(*start_routine)(void *), 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;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
 	int ret;
 
 	params = malloc(sizeof(*params));
@@ -219,15 +223,14 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params->start_routine = start_routine;
 	params->arg = arg;
-	params->refcnt = 2;
-
-	ret = pthread_barrier_init(&params->configured, NULL, 2);
-	if (ret != 0)
-		goto fail_no_barrier;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
 	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
-	if (ret != 0)
-		goto fail_with_barrier;
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
 
 	if (name != NULL) {
 		ret = rte_thread_setname(*thread, name);
@@ -236,24 +239,24 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 				"Cannot set name for ctrl thread\n");
 	}
 
-	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
-	if (ret != 0)
-		params->start_routine = NULL;
-
-	pthread_barrier_wait(&params->configured);
-	ctrl_params_free(params);
+	/* 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);
+	}
 
-	if (ret != 0)
-		/* start_routine has been set to NULL above; */
-		/* ctrl thread will exit immediately */
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
 		pthread_join(*thread, NULL);
+	}
 
-	return -ret;
-
-fail_with_barrier:
-	(void)pthread_barrier_destroy(&params->configured);
-
-fail_no_barrier:
+	ret = params->ret;
 	free(params);
 
 	return -ret;
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 1550b75da0..b81c8b2685 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -420,10 +420,11 @@ rte_thread_unregister(void);
 /**
  * Create a control thread.
  *
- * Wrapper to pthread_create(), pthread_setname_np() and
- * pthread_setaffinity_np(). 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.
+ * 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.
-- 
2.25.1

[dpdk-dev] [PATCH v5 2/2] test/eal: add a test for rte_ctrl_thread_create

[Honnappa Nagarahalli]

Add a testcase to test launching of control threads.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 app/test/test_lcores.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index 19a7ab9fce..35c47d5372 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -340,6 +340,44 @@ test_non_eal_lcores_callback(unsigned int eal_threads_count)
 	return -1;
 }
 
+static void *ctrl_thread_loop(void *arg)
+{
+	struct thread_context *t = arg;
+
+	printf("Control thread running successfully\n");
+
+	/* Set the thread state to DONE */
+	t->state = DONE;
+
+	return NULL;
+}
+
+static int
+test_ctrl_thread(void)
+{
+	struct thread_context ctrl_thread_context;
+	struct thread_context *t;
+
+	/* Create one control thread */
+	t = &ctrl_thread_context;
+	t->state = INIT;
+	if (rte_ctrl_thread_create(&t->id, "test_ctrl_threads",
+					NULL, ctrl_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.
+	 */
+	pthread_join(t->id, NULL);
+
+	/* Check if the control thread set the correct state */
+	if (t->state != DONE)
+		return -1;
+
+	return 0;
+}
+
 static int
 test_lcores(void)
 {
@@ -367,6 +405,9 @@ test_lcores(void)
 	if (test_non_eal_lcores_callback(eal_threads_count) < 0)
 		return TEST_FAILED;
 
+	if (test_ctrl_thread() < 0)
+		return TEST_FAILED;
+
 	return TEST_SUCCESS;
 }
 
-- 
2.25.1

Re: [dpdk-dev] [PATCH v5 2/2] test/eal: add a test for rte_ctrl_thread_create

[Olivier Matz]

On Thu, Oct 21, 2021 at 04:32:21PM -0500, Honnappa Nagarahalli wrote:
> Add a testcase to test launching of control threads.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v5 2/2] test/eal: add a test for rte_ctrl_thread_create

[David Marchand]

On Fri, Oct 22, 2021 at 10:35 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Thu, Oct 21, 2021 at 04:32:21PM -0500, Honnappa Nagarahalli wrote:
> > Add a testcase to test launching of control threads.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Series applied, thanks.


-- 
David Marchand