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

[Olivier Matz <olivier.matz@6wind.com>]

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
>