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

[Tyler Retzlaff]

On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> On Mon,  5 Dec 2022 12:24:26 -0800
> Tyler Retzlaff <roret...@linux.microsoft.com> wrote:
> 
> > Signed-off-by: Tyler Retzlaff <roret...@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.