Re: [PATCH v5 2/2] eal: fix failure path race setting new thread affinity

[David Marchand <david.marchand@redhat.com>]

On Thu, Mar 16, 2023 at 1:07 AM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> In rte_thread_create setting affinity after pthread_create may fail.
> Such a failure should result in the entire rte_thread_create failing
> but doesn't.
>
> Additionally if there is a failure to set affinity a race exists where
> the creating thread will free ctx and depending on scheduling of the new
> thread it may also free ctx (double free).
>
> Resolve the above by setting the affinity from the newly created thread
> using a condition variable to signal the completion of the thread
> start wrapper having completed.
>
> Since we are now waiting for the thread start wrapper to complete we can
> allocate the thread start wrapper context on the stack. While here clean
> up the variable naming in the context to better highlight the fields of
> the context require synchronization between the creating and created
> thread.
>
> Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/unix/rte_thread.c | 70 +++++++++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/lib/eal/unix/rte_thread.c b/lib/eal/unix/rte_thread.c
> index 37ebfcf..5992b04 100644
> --- a/lib/eal/unix/rte_thread.c
> +++ b/lib/eal/unix/rte_thread.c
> @@ -16,9 +16,14 @@ struct eal_tls_key {
>         pthread_key_t thread_index;
>  };
>
> -struct thread_routine_ctx {
> +struct thread_start_context {
>         rte_thread_func thread_func;
> -       void *routine_args;
> +       void *thread_args;
> +       const rte_thread_attr_t *thread_attr;
> +       pthread_mutex_t wrapper_mutex;
> +       pthread_cond_t wrapper_cond;
> +       int wrapper_ret;
> +       volatile int wrapper_done;

One question.

I see that wrapper_done is accessed under wrapper_mutex.
Is volatile needed?

(nit: a boolean is probably enough too)

I was thinking of squashing below diff:

diff --git a/lib/eal/unix/rte_thread.c b/lib/eal/unix/rte_thread.c
index 5992b04a45..5ab5267ca3 100644
--- a/lib/eal/unix/rte_thread.c
+++ b/lib/eal/unix/rte_thread.c
@@ -23,7 +23,7 @@ struct thread_start_context {
        pthread_mutex_t wrapper_mutex;
        pthread_cond_t wrapper_cond;
        int wrapper_ret;
-       volatile int wrapper_done;
+       bool wrapper_done;
 };

 static int
@@ -101,7 +101,7 @@ thread_start_wrapper(void *arg)

        pthread_mutex_lock(&ctx->wrapper_mutex);
        ctx->wrapper_ret = ret;
-       ctx->wrapper_done = 1;
+       ctx->wrapper_done = true;
        pthread_cond_signal(&ctx->wrapper_cond);
        pthread_mutex_unlock(&ctx->wrapper_mutex);

@@ -127,6 +127,7 @@ rte_thread_create(rte_thread_t *thread_id,
                .thread_func = thread_func,
                .thread_args = args,
                .thread_attr = thread_attr,
+               .wrapper_done = false,
                .wrapper_mutex = PTHREAD_MUTEX_INITIALIZER,
                .wrapper_cond = PTHREAD_COND_INITIALIZER,
        };
@@ -151,7 +152,6 @@ rte_thread_create(rte_thread_t *thread_id,
                        goto cleanup;
                }

-
                if (thread_attr->priority ==
                                RTE_THREAD_PRIORITY_REALTIME_CRITICAL) {
                        ret = ENOTSUP;
@@ -183,7 +183,7 @@ rte_thread_create(rte_thread_t *thread_id,
        }

        pthread_mutex_lock(&ctx.wrapper_mutex);
-       while (ctx.wrapper_done != 1)
+       while (!ctx.wrapper_done)
                pthread_cond_wait(&ctx.wrapper_cond, &ctx.wrapper_mutex);
        ret = ctx.wrapper_ret;
        pthread_mutex_unlock(&ctx.wrapper_mutex);


The rest lgtmn thanks Tyler.



-- 
David Marchand