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 0FB2641D44; Wed, 1 Mar 2023 09:11:59 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B3BBA4114B; Wed, 1 Mar 2023 09:11:58 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 9CF214114A for ; Wed, 1 Mar 2023 09:11:57 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677658317; 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: in-reply-to:in-reply-to:references:references; bh=MFfIKBjMS1Tpo422Bh0xcXbLi1q2POWqChFRqh3lNuQ=; b=EgECWBnC4LQv3xdWN6+eqFFoZurNkbsA6p6nQ7PEwTuN8/XGk4/3zWc5rF46/t4eG47XOq CqkIcb4t4WkbIYfy4/V0IK50e5PUiJSKMx1o9yFWHYcWUAtzR5MVYoFzfTdqbnhMHpbrol BhokO91yKTD59gckLbhTdXgSlR7F0uc= Received: from mail-pf1-f200.google.com (mail-pf1-f200.google.com [209.85.210.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-375-qSlFfdhmO7STLBwMn4rlyA-1; Wed, 01 Mar 2023 03:11:56 -0500 X-MC-Unique: qSlFfdhmO7STLBwMn4rlyA-1 Received: by mail-pf1-f200.google.com with SMTP id f14-20020a056a00228e00b005e46dd41b67so6410450pfe.8 for ; Wed, 01 Mar 2023 00:11:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677658315; h=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=MFfIKBjMS1Tpo422Bh0xcXbLi1q2POWqChFRqh3lNuQ=; b=N47w+2mhxxFIB2D/bUoVJdueZaHM5K7v75KZndelqaiE68GdHkaIGw5tazZzCmQanm mEs0ZYq6sIMNjJmu0qALirs0YANLxPwygcD2kVqUPg9FyuTZQ3EMgw+81+6mE0deZtQb 51LRrmblcHnEuUQvIS9wenP1V8nCNZ99kIM5OZVBAouqQOP+xCgYIimf6/NmLiEfU695 K12k3kaNeS7ANn2JSwYetgZmlzEttCzJTQZpXre46M+1U72wkh6PWDMQGDZ6Zky+K7H/ j3Po0h/9nuMU29QqY7JZiiH+UDxhhx7CRNyVYnlPNBockLCFvT7U2BkSsC5wLk79qsYv XrCg== X-Gm-Message-State: AO0yUKWIihZNA4erh+VB49GsrvYVecpa7Uqa9tlXapMrJc/QrtSyld3u ETgS6ZprGFY8A8kTTYepj/HdMMGhdKT2Cm5CtyRF4EefV13kQdkZe/m2kZoFFpos4zhruDOK6ws gwiye8LZ1ALa+7bIZRM4= X-Received: by 2002:a63:6e4c:0:b0:503:7be3:e81d with SMTP id j73-20020a636e4c000000b005037be3e81dmr1511278pgc.1.1677658314957; Wed, 01 Mar 2023 00:11:54 -0800 (PST) X-Google-Smtp-Source: AK7set+fRkxA+ZCmn3vfvrAFqSw9Ym8stk+TnnpHlhiM6do24TstU80STkECHf+50Itefwn74Gg+dTbAfork2S77xTU= X-Received: by 2002:a63:6e4c:0:b0:503:7be3:e81d with SMTP id j73-20020a636e4c000000b005037be3e81dmr1511272pgc.1.1677658314651; Wed, 01 Mar 2023 00:11:54 -0800 (PST) MIME-Version: 1.0 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> In-Reply-To: <1664989651-29303-3-git-send-email-roretzla@linux.microsoft.com> From: David Marchand Date: Wed, 1 Mar 2023 09:11:43 +0100 Message-ID: Subject: Re: [PATCH v5 2/6] eal: add thread lifetime management To: Tyler Retzlaff Cc: dev@dpdk.org, thomas@monjalon.net, dmitry.kozliuk@gmail.com, anatoly.burakov@intel.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" 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 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. 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 have the same concerns with the windows implementation. -- David Marchand