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 EFBD641DAE; Wed, 1 Mar 2023 21:34:06 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D37854114B; Wed, 1 Mar 2023 21:34:06 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 5E29E40693 for ; Wed, 1 Mar 2023 21:34:05 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 5EE6D20B9C3D; Wed, 1 Mar 2023 12:34:04 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 5EE6D20B9C3D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1677702844; bh=xJ4hw7FlX0a/OCy73fbl2ERDsbbyR/4C5mkik16umXY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=pVtxTM3YLyf8Lif+4UEptCumOq+2jGW0Qhf+WQNwjlM/Ci4Ryf1wQOHYuO6Y2q1E7 Z5Xe3SXjh9U/VnDfEi2Pw7/hR0z/GFtbgJeyfFRBGhS7hdwngFNXlMOegUBC/1K9Gh o3vnqGrrFbWUXglUHMVDM9ON+RpTv/grfwXSw4tI= Date: Wed, 1 Mar 2023 12:34:04 -0800 From: Tyler Retzlaff To: David Marchand Cc: dev@dpdk.org, thomas@monjalon.net, dmitry.kozliuk@gmail.com, anatoly.burakov@intel.com Subject: Re: [PATCH v5 2/6] eal: add thread lifetime management Message-ID: <20230301203404.GB29219@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1654783134-13303-1-git-send-email-roretzla@linux.microsoft.com> <1664989651-29303-1-git-send-email-roretzla@linux.microsoft.com> <1664989651-29303-3-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 Wed, Mar 01, 2023 at 09:11:43AM +0100, David Marchand wrote: > Hello Tyler, > > On Wed, Oct 5, 2022 at 7:07 PM Tyler Retzlaff > wrote: > > diff --git a/lib/eal/unix/rte_thread.c b/lib/eal/unix/rte_thread.c > > index 9126595..d4c1a7f 100644 > > --- a/lib/eal/unix/rte_thread.c > > +++ b/lib/eal/unix/rte_thread.c > > @@ -16,6 +16,11 @@ struct eal_tls_key { > > pthread_key_t thread_index; > > }; > > > > +struct thread_routine_ctx { > > + rte_thread_func thread_func; > > + void *routine_args; > > +}; > > + > > static int > > thread_map_priority_to_os_value(enum rte_thread_priority eal_pri, int *os_pri, > > int *pol) > > @@ -75,6 +80,136 @@ struct eal_tls_key { > > return 0; > > } > > > > +static void * > > +thread_func_wrapper(void *arg) > > +{ > > + struct thread_routine_ctx ctx = *(struct thread_routine_ctx *)arg; > > + > > + free(arg); > > + > > + return (void *)(uintptr_t)ctx.thread_func(ctx.routine_args); > > +} > > + > > +int > > +rte_thread_create(rte_thread_t *thread_id, > > + const rte_thread_attr_t *thread_attr, > > + rte_thread_func thread_func, void *args) > > +{ > > + int ret = 0; > > + pthread_attr_t attr; > > + pthread_attr_t *attrp = NULL; > > + struct thread_routine_ctx *ctx; > > + struct sched_param param = { > > + .sched_priority = 0, > > + }; > > + int policy = SCHED_OTHER; > > + > > + ctx = calloc(1, sizeof(*ctx)); > > + if (ctx == NULL) { > > + RTE_LOG(DEBUG, EAL, "Insufficient memory for thread context allocations\n"); > > + ret = ENOMEM; > > + goto cleanup; > > + } > > + ctx->routine_args = args; > > + ctx->thread_func = thread_func; > > + > > + if (thread_attr != NULL) { > > + ret = pthread_attr_init(&attr); > > + if (ret != 0) { > > + RTE_LOG(DEBUG, EAL, "pthread_attr_init failed\n"); > > + goto cleanup; > > + } > > + > > + attrp = &attr; > > + > > + /* > > + * Set the inherit scheduler parameter to explicit, > > + * otherwise the priority attribute is ignored. > > + */ > > + ret = pthread_attr_setinheritsched(attrp, > > + PTHREAD_EXPLICIT_SCHED); > > + if (ret != 0) { > > + RTE_LOG(DEBUG, EAL, "pthread_attr_setinheritsched failed\n"); > > + goto cleanup; > > + } > > + > > + > > + if (thread_attr->priority == > > + RTE_THREAD_PRIORITY_REALTIME_CRITICAL) { > > + ret = ENOTSUP; > > + goto cleanup; > > + } > > + ret = thread_map_priority_to_os_value(thread_attr->priority, > > + ¶m.sched_priority, &policy); > > + if (ret != 0) > > + goto cleanup; > > + > > + ret = pthread_attr_setschedpolicy(attrp, policy); > > + if (ret != 0) { > > + RTE_LOG(DEBUG, EAL, "pthread_attr_setschedpolicy failed\n"); > > + goto cleanup; > > + } > > + > > + ret = pthread_attr_setschedparam(attrp, ¶m); > > + if (ret != 0) { > > + RTE_LOG(DEBUG, EAL, "pthread_attr_setschedparam failed\n"); > > + goto cleanup; > > + } > > + } > > + > > + ret = pthread_create((pthread_t *)&thread_id->opaque_id, attrp, > > + thread_func_wrapper, ctx); > > + if (ret != 0) { > > + RTE_LOG(DEBUG, EAL, "pthread_create failed\n"); > > + goto cleanup; > > + } > > + > > + if (thread_attr != NULL && CPU_COUNT(&thread_attr->cpuset) > 0) { > > + ret = rte_thread_set_affinity_by_id(*thread_id, > > + &thread_attr->cpuset); > > + if (ret != 0) { > > + RTE_LOG(DEBUG, EAL, "rte_thread_set_affinity_by_id failed\n"); > > + goto cleanup; > > + } > > + } > > + > > + ctx = NULL; > > +cleanup: > > + free(ctx); > > + if (attrp != NULL) > > + pthread_attr_destroy(&attr); > > + > > + return ret; > > +} > > I am looking back at potential races in our code while reviewing the > ctrl thread creation fix, and I stopped at this patch. Thank you for doing this, this series I actually did not start I only took over so the review is welcome. > > What happens if rte_thread_set_affinity_by_id() fails? > A new thread got started, but I don't see it being terminated in the > cleanup phase. > > > In this same situation, we may have a small race for a double free on > ctx since thread_func_wrapper() may have already been called from the > new thread. I will review this and get back with fixes if necessary. > > > I have the same concerns with the windows implementation. I will check it too. > > > -- > David Marchand