[RFC] eal: simplify the implementation of rte_ctrl_thread_create

  The current described behaviour of rte_ctrl_thread_create is
rigid which makes the implementation of the function complex.
The behavior is abstracted to allow for simplified implementation.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
 lib/eal/include/rte_lcore.h        |  8 ++--
 2 files changed, 32 insertions(+), 41 deletions(-)
  

Hi Honnappa,

(Sorry if you get this message twice, I forgot to reply all the first time)

Sorry for the late reply. I was also away.

I have only made one small contribution to DPDK so I'll defer to
others to decide whether this patch should be accepted. When I
submitted my patch, I got the feeling that busy loops were somewhat
frowned upon but it might be a good solution here.

Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> a écrit :
<snip>
> +       /* Synchronization variable between the control thread
> +        * and the thread calling rte_ctrl_thread_create function.
> +        * 0 - Initialized
> +        * 1 - Control thread is running successfully
> +        * 2 - Control thread encountered an error. 'ret' has the
> +        *     error code.
> +        */
> +       unsigned int sync;

This is highly subjective but the name of this variable doesn't
clearly indicate that once it is set, the control thread is
effectively done with the rte_thread_ctrl_params. The downside that I
can see (as opposed to the previous reference counting approach) is
that anyone making further changes would have to be cognizant of that
fact. Otherwise, any new code that accesses "sync" in the control
thread, after "sync" has been set, would create a race.

<snip>
>         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>         if (ret != 0)
> -               goto fail_with_barrier;
> +               goto thread_create_failed;

This is also highly subjective but since the goto label is only used
here, you could completely get rid of the goto and simply free and
return here.

<snip>
> +       /* Wait for the control thread to initialize successfully */
> +       while (!params->sync)
> +               rte_pause();
> +       ret = params->ret;
>
> -       pthread_barrier_wait(&params->configured);
> -       ctrl_params_free(params);
> +       free(params);

Maybe I'm missing something but it seems like you're accessing
params->sync after this line (use after free). I assume you meant to
add this line after the subsequent if block?

Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> a écrit :
>
> The current described behaviour of rte_ctrl_thread_create is
> rigid which makes the implementation of the function complex.
> The behavior is abstracted to allow for simplified implementation.
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
>  lib/eal/include/rte_lcore.h        |  8 ++--
>  2 files changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index 1a52f42a2b..86cacd840c 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -169,35 +169,35 @@ __rte_thread_uninit(void)
>  struct rte_thread_ctrl_params {
>         void *(*start_routine)(void *);
>         void *arg;
> -       pthread_barrier_t configured;
> -       unsigned int refcnt;
> +       int ret;
> +       /* Synchronization variable between the control thread
> +        * and the thread calling rte_ctrl_thread_create function.
> +        * 0 - Initialized
> +        * 1 - Control thread is running successfully
> +        * 2 - Control thread encountered an error. 'ret' has the
> +        *     error code.
> +        */
> +       unsigned int sync;
>  };
>
> -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> -{
> -       if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
> -               (void)pthread_barrier_destroy(&params->configured);
> -               free(params);
> -       }
> -}
> -
>  static void *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 *);
> +       void *(*start_routine)(void *) = params->start_routine;
>         void *routine_arg = params->arg;
>
>         __rte_thread_init(rte_lcore_id(), cpuset);
> -
> -       pthread_barrier_wait(&params->configured);
> -       start_routine = params->start_routine;
> -       ctrl_params_free(params);
> -
> -       if (start_routine == NULL)
> +       params->ret = pthread_setaffinity_np(pthread_self(),
> +                               sizeof(*cpuset), cpuset);
> +       if (params->ret != 0) {
> +               params->sync = 2;
>                 return NULL;
> +       }
> +
> +       params->sync = 1;
>
>         return start_routine(routine_arg);
>  }
> @@ -207,9 +207,6 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>                 const pthread_attr_t *attr,
>                 void *(*start_routine)(void *), 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;
>         int ret;
>
> @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
>
>         params->start_routine = start_routine;
>         params->arg = arg;
> -       params->refcnt = 2;
> -
> -       ret = pthread_barrier_init(&params->configured, NULL, 2);
> -       if (ret != 0)
> -               goto fail_no_barrier;
> +       params->ret = 0;
> +       params->sync = 0;
>
>         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>         if (ret != 0)
> -               goto fail_with_barrier;
> +               goto thread_create_failed;
>
>         if (name != NULL) {
>                 ret = rte_thread_setname(*thread, name);
> @@ -236,24 +230,21 @@ 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;
> +       /* Wait for the control thread to initialize successfully */
> +       while (!params->sync)
> +               rte_pause();
> +       ret = params->ret;
>
> -       pthread_barrier_wait(&params->configured);
> -       ctrl_params_free(params);
> +       free(params);
>
> -       if (ret != 0)
> -               /* start_routine has been set to NULL above; */
> -               /* ctrl thread will exit immediately */
> +       if (params->sync != 1)
> +               /* ctrl thread is exiting */
>                 pthread_join(*thread, NULL);
>
>         return -ret;
>
> -fail_with_barrier:
> -       (void)pthread_barrier_destroy(&params->configured);
> +thread_create_failed:
>
> -fail_no_barrier:
>         free(params);
>
>         return -ret;
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 1550b75da0..f1cc5e38dc 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -420,10 +420,10 @@ rte_thread_unregister(void);
>  /**
>   * Create a control thread.
>   *
> - * Wrapper to pthread_create(), pthread_setname_np() and
> - * pthread_setaffinity_np(). The affinity of the new thread is based
> - * on the CPU affinity retrieved at the time rte_eal_init() was called,
> - * the dataplane and service lcores are then excluded.
> + * Creates a control thread with the given name and attributes. The
> + * affinity of the new thread is based on the CPU affinity retrieved
> + * at the time rte_eal_init() was called, the dataplane and service
> + * lcores are then excluded.
>   *
>   * @param thread
>   *   Filled with the thread id of the new created thread.
> --
> 2.17.1
>
  

<snip>

> 
> Hi Honnappa,
> 
> (Sorry if you get this message twice, I forgot to reply all the first time)
> 
> Sorry for the late reply. I was also away.
No problem, thanks for your comments.

> 
> I have only made one small contribution to DPDK so I'll defer to others to
> decide whether this patch should be accepted. When I submitted my patch, I
> got the feeling that busy loops were somewhat frowned upon but it might be
> a good solution here.
IMO, it should be ok here since the busy loop is for a small period. Adding sched_yield should help reduce the actual spinning.

> 
> Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
> <honnappa.nagarahalli@arm.com> a écrit :
> <snip>
> > +       /* Synchronization variable between the control thread
> > +        * and the thread calling rte_ctrl_thread_create function.
> > +        * 0 - Initialized
> > +        * 1 - Control thread is running successfully
> > +        * 2 - Control thread encountered an error. 'ret' has the
> > +        *     error code.
> > +        */
> > +       unsigned int sync;
> 
> This is highly subjective but the name of this variable doesn't clearly indicate
> that once it is set, the control thread is effectively done with the
> rte_thread_ctrl_params. The downside that I can see (as opposed to the
> previous reference counting approach) is that anyone making further changes
> would have to be cognizant of that fact. Otherwise, any new code that
> accesses "sync" in the control thread, after "sync" has been set, would create
> a race.
May be "ctrl_thread_status" would be better.

> 
> <snip>
> >         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> >         if (ret != 0)
> > -               goto fail_with_barrier;
> > +               goto thread_create_failed;
> 
> This is also highly subjective but since the goto label is only used here, you
> could completely get rid of the goto and simply free and return here.
Thanks, agree.

> 
> <snip>
> > +       /* Wait for the control thread to initialize successfully */
> > +       while (!params->sync)
> > +               rte_pause();
> > +       ret = params->ret;
> >
> > -       pthread_barrier_wait(&params->configured);
> > -       ctrl_params_free(params);
> > +       free(params);
> 
> Maybe I'm missing something but it seems like you're accessing
> params->sync after this line (use after free). I assume you meant to
> add this line after the subsequent if block?
You are correct. I realized this yesterday while incorporating review comments. That made me think, we need a test case. I am adding a test case as well.

> 
> Le ven. 30 juil. 2021 à 17:37, Honnappa Nagarahalli
> <honnappa.nagarahalli@arm.com> a écrit :
> >
> > The current described behaviour of rte_ctrl_thread_create is rigid
> > which makes the implementation of the function complex.
> > The behavior is abstracted to allow for simplified implementation.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> >  lib/eal/common/eal_common_thread.c | 65 +++++++++++++-----------------
> >  lib/eal/include/rte_lcore.h        |  8 ++--
> >  2 files changed, 32 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/eal/common/eal_common_thread.c
> > b/lib/eal/common/eal_common_thread.c
> > index 1a52f42a2b..86cacd840c 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -169,35 +169,35 @@ __rte_thread_uninit(void)  struct
> > rte_thread_ctrl_params {
> >         void *(*start_routine)(void *);
> >         void *arg;
> > -       pthread_barrier_t configured;
> > -       unsigned int refcnt;
> > +       int ret;
> > +       /* Synchronization variable between the control thread
> > +        * and the thread calling rte_ctrl_thread_create function.
> > +        * 0 - Initialized
> > +        * 1 - Control thread is running successfully
> > +        * 2 - Control thread encountered an error. 'ret' has the
> > +        *     error code.
> > +        */
> > +       unsigned int sync;
> >  };
> >
> > -static void ctrl_params_free(struct rte_thread_ctrl_params *params)
> > -{
> > -       if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) ==
> 0) {
> > -               (void)pthread_barrier_destroy(&params->configured);
> > -               free(params);
> > -       }
> > -}
> > -
> >  static void *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 *);
> > +       void *(*start_routine)(void *) = params->start_routine;
> >         void *routine_arg = params->arg;
> >
> >         __rte_thread_init(rte_lcore_id(), cpuset);
> > -
> > -       pthread_barrier_wait(&params->configured);
> > -       start_routine = params->start_routine;
> > -       ctrl_params_free(params);
> > -
> > -       if (start_routine == NULL)
> > +       params->ret = pthread_setaffinity_np(pthread_self(),
> > +                               sizeof(*cpuset), cpuset);
> > +       if (params->ret != 0) {
> > +               params->sync = 2;
> >                 return NULL;
> > +       }
> > +
> > +       params->sync = 1;
> >
> >         return start_routine(routine_arg);  } @@ -207,9 +207,6 @@
> > rte_ctrl_thread_create(pthread_t *thread, const char *name,
> >                 const pthread_attr_t *attr,
> >                 void *(*start_routine)(void *), 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;
> >         int ret;
> >
> > @@ -219,15 +216,12 @@ rte_ctrl_thread_create(pthread_t *thread, const
> > char *name,
> >
> >         params->start_routine = start_routine;
> >         params->arg = arg;
> > -       params->refcnt = 2;
> > -
> > -       ret = pthread_barrier_init(&params->configured, NULL, 2);
> > -       if (ret != 0)
> > -               goto fail_no_barrier;
> > +       params->ret = 0;
> > +       params->sync = 0;
> >
> >         ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
> >         if (ret != 0)
> > -               goto fail_with_barrier;
> > +               goto thread_create_failed;
> >
> >         if (name != NULL) {
> >                 ret = rte_thread_setname(*thread, name); @@ -236,24
> > +230,21 @@ 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;
> > +       /* Wait for the control thread to initialize successfully */
> > +       while (!params->sync)
> > +               rte_pause();
> > +       ret = params->ret;
> >
> > -       pthread_barrier_wait(&params->configured);
> > -       ctrl_params_free(params);
> > +       free(params);
> >
> > -       if (ret != 0)
> > -               /* start_routine has been set to NULL above; */
> > -               /* ctrl thread will exit immediately */
> > +       if (params->sync != 1)
> > +               /* ctrl thread is exiting */
> >                 pthread_join(*thread, NULL);
> >
> >         return -ret;
> >
> > -fail_with_barrier:
> > -       (void)pthread_barrier_destroy(&params->configured);
> > +thread_create_failed:
> >
> > -fail_no_barrier:
> >         free(params);
> >
> >         return -ret;
> > diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> > index 1550b75da0..f1cc5e38dc 100644
> > --- a/lib/eal/include/rte_lcore.h
> > +++ b/lib/eal/include/rte_lcore.h
> > @@ -420,10 +420,10 @@ rte_thread_unregister(void);
> >  /**
> >   * Create a control thread.
> >   *
> > - * Wrapper to pthread_create(), pthread_setname_np() and
> > - * pthread_setaffinity_np(). The affinity of the new thread is based
> > - * on the CPU affinity retrieved at the time rte_eal_init() was
> > called,
> > - * the dataplane and service lcores are then excluded.
> > + * Creates a control thread with the given name and attributes. The
> > + * affinity of the new thread is based on the CPU affinity retrieved
> > + * at the time rte_eal_init() was called, the dataplane and service
> > + * lcores are then excluded.
> >   *
> >   * @param thread
> >   *   Filled with the thread id of the new created thread.
> > --
> > 2.17.1
> >