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.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.
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