[v4,1/2] eal: simplify the implementation of rte_ctrl_thread_create
Checks
Commit Message
Remove the usage of pthread barrier and replace it with
synchronization using atomic variable. This also removes
the use of reference count required to synchronize freeing
the memory.
Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
v4:
1) Used rte_delay_us_sleep instead of sched_yield. Using sched_yield
requires to maintain a Windows version of the rte_ctrl_thread_create
API.
v3:
1) RFC to Patch
2) Changed commit log and API description [Olivier]
3) Used sched_yield while spinning [Stephen]
4) Fixed use after free
5) Added a test case
v2:
1) Used compiler's C++11 atomic built-ins to access the shared variable.
lib/eal/common/eal_common_thread.c | 87 +++++++++++++++---------------
lib/eal/include/rte_lcore.h | 9 ++--
2 files changed, 49 insertions(+), 47 deletions(-)
Comments
Hi Honnappa,
On Wed, Oct 13, 2021 at 03:18:10PM -0500, Honnappa Nagarahalli wrote:
> Remove the usage of pthread barrier and replace it with
> synchronization using atomic variable. This also removes
> the use of reference count required to synchronize freeing
> the memory.
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
Few cosmetics comments below.
(...)
> +enum __rte_ctrl_thread_status {
> + __RTE_CTRL_THREAD_LAUNCHING, /* Yet to call pthread_create function */
> + __RTE_CTRL_THREAD_RUNNING, /* Control thread is running successfully */
> + __RTE_CTRL_THREAD_ERROR /* Control thread encountered an error */
> +};
> +
Are there double underscores needed?
I think even the rte_ prefix could be removed since this is not exposed.
(...)
> @@ -236,24 +239,22 @@ 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;
> -
> - pthread_barrier_wait(¶ms->configured);
> - ctrl_params_free(params);
> -
> - if (ret != 0)
> - /* start_routine has been set to NULL above; */
> - /* ctrl thread will exit immediately */
> + /* Wait for the control thread to initialize successfully */
> + while ((ctrl_thread_status =
> + __atomic_load_n(¶ms->ctrl_thread_status, __ATOMIC_ACQUIRE))
> + == __RTE_CTRL_THREAD_LAUNCHING)
> + /* Yield the CPU. Using sched_yield call requires maintaining
> + * another implementation for Windows as sched_yield is not
> + * supported on Windows.
> + */
> + rte_delay_us_sleep(1);
> +
> + /* Check if the control thread encountered an error */
> + if (ctrl_thread_status == __RTE_CTRL_THREAD_ERROR)
> + /* ctrl thread is exiting */
> pthread_join(*thread, NULL);
While not required, I suggest to use curly brackets when the number of lines
in the body of the loop or test (including comments).
Thanks
Olivier
<snip>
>
> Hi Honnappa,
>
> On Wed, Oct 13, 2021 at 03:18:10PM -0500, Honnappa Nagarahalli wrote:
> > Remove the usage of pthread barrier and replace it with
> > synchronization using atomic variable. This also removes the use of
> > reference count required to synchronize freeing the memory.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
>
> Few cosmetics comments below.
>
> (...)
>
> > +enum __rte_ctrl_thread_status {
> > + __RTE_CTRL_THREAD_LAUNCHING, /* Yet to call pthread_create
> function */
> > + __RTE_CTRL_THREAD_RUNNING, /* Control thread is running
> successfully */
> > + __RTE_CTRL_THREAD_ERROR /* Control thread encountered an error
> */ };
> > +
>
> Are there double underscores needed?
> I think even the rte_ prefix could be removed since this is not exposed.
I am fine to change this, just want to be consistent with the rest of the code. Looks like a lot of code does not use __RTE.
>
> (...)
>
> > @@ -236,24 +239,22 @@ 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;
> > -
> > - pthread_barrier_wait(¶ms->configured);
> > - ctrl_params_free(params);
> > -
> > - if (ret != 0)
> > - /* start_routine has been set to NULL above; */
> > - /* ctrl thread will exit immediately */
> > + /* Wait for the control thread to initialize successfully */
> > + while ((ctrl_thread_status =
> > + __atomic_load_n(¶ms->ctrl_thread_status,
> __ATOMIC_ACQUIRE))
> > + == __RTE_CTRL_THREAD_LAUNCHING)
> > + /* Yield the CPU. Using sched_yield call requires maintaining
> > + * another implementation for Windows as sched_yield is not
> > + * supported on Windows.
> > + */
> > + rte_delay_us_sleep(1);
> > +
> > + /* Check if the control thread encountered an error */
> > + if (ctrl_thread_status == __RTE_CTRL_THREAD_ERROR)
> > + /* ctrl thread is exiting */
> > pthread_join(*thread, NULL);
>
> While not required, I suggest to use curly brackets when the number of lines in
> the body of the loop or test (including comments).
ok
>
>
> Thanks
> Olivier
@@ -166,38 +166,44 @@ __rte_thread_uninit(void)
RTE_PER_LCORE(_lcore_id) = LCORE_ID_ANY;
}
+enum __rte_ctrl_thread_status {
+ __RTE_CTRL_THREAD_LAUNCHING, /* Yet to call pthread_create function */
+ __RTE_CTRL_THREAD_RUNNING, /* Control thread is running successfully */
+ __RTE_CTRL_THREAD_ERROR /* Control thread encountered an error */
+};
+
struct rte_thread_ctrl_params {
void *(*start_routine)(void *);
void *arg;
- pthread_barrier_t configured;
- unsigned int refcnt;
+ int ret;
+ /* Control thread status. If the status is __RTE_CTRL_THREAD_ERROR
+ * 'ret' has the error code.
+ */
+ enum __rte_ctrl_thread_status ctrl_thread_status;
};
-static void ctrl_params_free(struct rte_thread_ctrl_params *params)
-{
- if (__atomic_sub_fetch(¶ms->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
- (void)pthread_barrier_destroy(¶ms->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(¶ms->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) {
+ __atomic_store_n(¶ms->ctrl_thread_status,
+ __RTE_CTRL_THREAD_ERROR,
+ __ATOMIC_RELEASE);
return NULL;
+ }
+
+ __atomic_store_n(¶ms->ctrl_thread_status,
+ __RTE_CTRL_THREAD_RUNNING,
+ __ATOMIC_RELEASE);
return start_routine(routine_arg);
}
@@ -207,10 +213,8 @@ 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;
+ enum __rte_ctrl_thread_status ctrl_thread_status;
int ret;
params = malloc(sizeof(*params));
@@ -219,15 +223,14 @@ 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(¶ms->configured, NULL, 2);
- if (ret != 0)
- goto fail_no_barrier;
+ params->ret = 0;
+ params->ctrl_thread_status = __RTE_CTRL_THREAD_LAUNCHING;
ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
- if (ret != 0)
- goto fail_with_barrier;
+ if (ret != 0) {
+ free(params);
+ return -ret;
+ }
if (name != NULL) {
ret = rte_thread_setname(*thread, name);
@@ -236,24 +239,22 @@ 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;
-
- pthread_barrier_wait(¶ms->configured);
- ctrl_params_free(params);
-
- if (ret != 0)
- /* start_routine has been set to NULL above; */
- /* ctrl thread will exit immediately */
+ /* Wait for the control thread to initialize successfully */
+ while ((ctrl_thread_status =
+ __atomic_load_n(¶ms->ctrl_thread_status, __ATOMIC_ACQUIRE))
+ == __RTE_CTRL_THREAD_LAUNCHING)
+ /* Yield the CPU. Using sched_yield call requires maintaining
+ * another implementation for Windows as sched_yield is not
+ * supported on Windows.
+ */
+ rte_delay_us_sleep(1);
+
+ /* Check if the control thread encountered an error */
+ if (ctrl_thread_status == __RTE_CTRL_THREAD_ERROR)
+ /* ctrl thread is exiting */
pthread_join(*thread, NULL);
- return -ret;
-
-fail_with_barrier:
- (void)pthread_barrier_destroy(¶ms->configured);
-
-fail_no_barrier:
+ ret = params->ret;
free(params);
return -ret;
@@ -420,10 +420,11 @@ 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. If setting the name of the thread fails,
+ * the error is ignored and a debug message is logged.
*
* @param thread
* Filled with the thread id of the new created thread.