From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7BD8641EBB; Fri, 17 Mar 2023 11:45:24 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5AF2B42F98; Fri, 17 Mar 2023 11:45:24 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id C365540395 for ; Fri, 17 Mar 2023 11:45:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1679049922; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mQ6Xt70//v10KwTwv5LN/h8Ys61rTK1TBYoH5uHrY9w=; b=frqE+8B4MorIm1ZV0xbfEd1yiqF+FdLlZ4WCQS3d+8jqKNqpRHEtuWsGJmVSa6daJ5JkkX kiB3CAiAnRh2K9JtLKt6bK89xZdPThaG8L8wDWpv6bibIFlwvXo0vubgjK53NWu/rxPd6J j7WxcJboG71dOgNbxGV4a+2P1rlhh9U= Received: from mail-pl1-f198.google.com (mail-pl1-f198.google.com [209.85.214.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-412-f2edwUhiMzqjOVaU3BCdTQ-1; Fri, 17 Mar 2023 06:45:21 -0400 X-MC-Unique: f2edwUhiMzqjOVaU3BCdTQ-1 Received: by mail-pl1-f198.google.com with SMTP id l1-20020a170903244100b001a0468b4afcso2587403pls.12 for ; Fri, 17 Mar 2023 03:45:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679049920; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=mQ6Xt70//v10KwTwv5LN/h8Ys61rTK1TBYoH5uHrY9w=; b=N6DxZiibbMyndFg+DIELpmI5zVkWy37+xMWq2F1PWdcQ8VVmJSX6nu2NFizRGE3+qy 1tMMu1xVx3CjSDAlXQnrJSQ1wMkDD7EV3fAx26BWAcfvFZexbeFFnu72fmm00zIrUSCJ +MMvfUL67i+wiOOrVIXNOxbxZwQJCDkCX6KuIQWDpivBSUhy5O1IXQbkaYK4AJZVajGP GpoMoty4B4VptHtyib+sfFjBbvIRSl4M8v4qwVALwTBxROcmIHMBfTnhZMG+ksac6cOK JX94x4Y7beGcNHhcng2OuerQijZvyh0C88fKwlPEPdvqA6zknxb5i2zLbcx94iXbk3xA cucQ== X-Gm-Message-State: AO0yUKWV2HKYJGVBR0yjTfuWPDTyYSFuVgWfa1jfbuCb7lrEnp4KXd5Q rPLE5t9eJuanlUUVScSwb7LC5Hgnw6n9yj1quvIWy5kbvTQu8+/Uz1b6acMYwUpzhEl80ddJTIV PVqwSiws/tTIsMZw3qpE= X-Received: by 2002:a17:90a:458f:b0:23b:2efa:44ed with SMTP id v15-20020a17090a458f00b0023b2efa44edmr2083169pjg.8.1679049920106; Fri, 17 Mar 2023 03:45:20 -0700 (PDT) X-Google-Smtp-Source: AK7set/Yh1dfsJ+BwzikDLlFrjF7etaQZKLd9WtFlwWWlwi24MZ4pRguQK5hfoE+9kYRK6TMRqDZvqCTsJDXkPGVFQA= X-Received: by 2002:a17:90a:458f:b0:23b:2efa:44ed with SMTP id v15-20020a17090a458f00b0023b2efa44edmr2083164pjg.8.1679049919798; Fri, 17 Mar 2023 03:45:19 -0700 (PDT) MIME-Version: 1.0 References: <1677782682-27200-1-git-send-email-roretzla@linux.microsoft.com> <1678925224-2706-1-git-send-email-roretzla@linux.microsoft.com> <1678925224-2706-3-git-send-email-roretzla@linux.microsoft.com> In-Reply-To: <1678925224-2706-3-git-send-email-roretzla@linux.microsoft.com> From: David Marchand Date: Fri, 17 Mar 2023 11:45:08 +0100 Message-ID: Subject: Re: [PATCH v5 2/2] eal: fix failure path race setting new thread affinity To: Tyler Retzlaff Cc: dev@dpdk.org, thomas@monjalon.net, stephen@networkplumber.org, stable@dpdk.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, Mar 16, 2023 at 1:07=E2=80=AFAM Tyler Retzlaff 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 > --- > 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 =3D ret; - ctx->wrapper_done =3D 1; + ctx->wrapper_done =3D 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 =3D thread_func, .thread_args =3D args, .thread_attr =3D thread_attr, + .wrapper_done =3D false, .wrapper_mutex =3D PTHREAD_MUTEX_INITIALIZER, .wrapper_cond =3D PTHREAD_COND_INITIALIZER, }; @@ -151,7 +152,6 @@ rte_thread_create(rte_thread_t *thread_id, goto cleanup; } - if (thread_attr->priority =3D=3D RTE_THREAD_PRIORITY_REALTIME_CRITICAL) { ret =3D ENOTSUP; @@ -183,7 +183,7 @@ rte_thread_create(rte_thread_t *thread_id, } pthread_mutex_lock(&ctx.wrapper_mutex); - while (ctx.wrapper_done !=3D 1) + while (!ctx.wrapper_done) pthread_cond_wait(&ctx.wrapper_cond, &ctx.wrapper_mutex); ret =3D ctx.wrapper_ret; pthread_mutex_unlock(&ctx.wrapper_mutex); The rest lgtmn thanks Tyler. --=20 David Marchand