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

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

On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> On Mon,  5 Dec 2022 12:24:26 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> >  lib/eal/version.map                |  3 ++
> >  3 files changed, 117 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > index c5d8b43..ca85c51 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >  
> >  struct rte_thread_ctrl_params {
> > -	void *(*start_routine)(void *);
> > +	union {
> > +		void * (*start_routine)(void *);
> > +		rte_thread_func thread_func;
> > +	} u;
> 
> Why not just use rte_thread_func, this in internal.
> 
> >  	void *arg;
> >  	int ret;
> >  	/* Control thread status.
> > @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
> >  	enum __rte_ctrl_thread_status ctrl_thread_status;
> >  };
> >  
> > -static void *ctrl_thread_init(void *arg)
> > +static int ctrl_thread_init(void *arg)
> >  {
> >  	struct internal_config *internal_conf =
> >  		eal_get_internal_configuration();
> >  	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
> >  	struct rte_thread_ctrl_params *params = arg;
> > -	void *(*start_routine)(void *) = params->start_routine;
> > -	void *routine_arg = params->arg;
> >  
> >  	__rte_thread_init(rte_lcore_id(), cpuset);
> >  	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
> > @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
> >  	if (params->ret != 0) {
> >  		__atomic_store_n(&params->ctrl_thread_status,
> >  			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> > -		return NULL;
> > +		return params->ret;
> >  	}
> >  
> >  	__atomic_store_n(&params->ctrl_thread_status,
> >  		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
> >  
> > -	return start_routine(routine_arg);
> > +	return 0;
> > +}
> > +
> > +static void *ctrl_thread_start(void *arg)
> > +{
> > +	struct rte_thread_ctrl_params *params = arg;
> > +	void *(*start_routine)(void *) = params->u.start_routine;
> > +
> > +	if (0 != ctrl_thread_init(arg))
> > +		return NULL;
> 
> DPDK uses the Linux not Windows coding style.
> Windows uses the constant on left side of comparison because of the common
> programming error of putting assignment where conditional was intended.
> Linux uses a compiler that puts out a warning, so the more natural
> style is action != result

yes, of course.

i will fix this along with another minor style problem picked up by the
CI.

thanks.