On Thu, Dec 08, 2022 at 10:59:13PM +0100, Mattias Rönnblom wrote:
> On 2022-12-07 17:38, Tyler Retzlaff wrote:
> >
> >>
> >>>+ } u;
> >>> void *arg;
> >>> int ret;
> >>
> >>Why is 'ret' needed? (This question is unrelated to your patch.)
> >
> >i'm not the original author so difficult to answer authoritatively. but
> >if i have to speculate i'd say it might be to work around the windows
> >pthread_join stub being implemented as a noop. specifically it didn't
> >communicate the return value from the start_routine.
> >
> >the recently added rte_thread_join addresses this (which
> >rte_control_thread_create uses) and could remove ret parameter and to
> >avoid touching the new function implementation in the future it can not
> >use ret now.
> >
> >i'll make this change in v3.
> >
> >for the original function i will leave the code as is to minimize the
> >diff. when rte_ctrl_thread_create is removed i'll make a note to
> >eliminate the ret parameter as well.
> >
>
> I would focus on minimizing the complexity of the end result, rather
> than the size of the patch.
agreed, that's why i decided to leave it as is. more change than is
needed right now and simpler to make after the old api is removed.
> >>>+ /* Wait for the control thread to initialize successfully */
> >>>+ while ((ctrl_thread_status =
> >>>+ __atomic_load_n(¶ms->ctrl_thread_status,
> >>>+ __ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
> >>>+ /* Yield the CPU. Using sched_yield call requires maintaining
> >>>+ * another implementation for Windows as sched_yield is not
> >>>+ * supported on Windows.
> >>>+ */
> >>
> >>sched_yield() also doesn't necessarily yield the CPU.
> >
> >copied from original code and understood, but are you requesting a
> >change here or just a comment correction? can you offer wording you
> >would find suitable?
> >
>
> I would just remove the comment.
sounds good, i'll remove it in the next rev.
thanks!
