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

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

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