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.nagaraha...@arm.com>

Reviewed-by: Olivier Matz <olivier.m...@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