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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<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