Re: [PATCH v2 1/3] eal: add rte control thread create API

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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(&params->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!