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 1D838A0032; Sat, 18 Jun 2022 14:59:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 43F4140F35; Sat, 18 Jun 2022 14:59:13 +0200 (CEST) Received: from mail-lj1-f169.google.com (mail-lj1-f169.google.com [209.85.208.169]) by mails.dpdk.org (Postfix) with ESMTP id 4E86340F19 for ; Sat, 18 Jun 2022 14:59:11 +0200 (CEST) Received: by mail-lj1-f169.google.com with SMTP id j22so724834ljg.0 for ; Sat, 18 Jun 2022 05:59:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=GQbBNeutQTO6ecrZYoPUyAu0VL/YjbG5ADPdI1TCfVg=; b=M2FAnfNixrunmjTOzaQgDY0Ul6xWe9RNv0PdOkw8PRLlhEoQivHQvtCjksxDXzM9po 1XCTAxqh2dyFDqLAFKAiXWrsCVgNJTronfhm8fIfRP/fREy2o26efEQevv1bprEu5Ngn duMlm9iodgyTe7jQ/4MLvFHnKkbeClj36Nbpcl77Fxb8ArpqfhgTKas+mKu0wi+XsElW oNfYRY5rFrlWOG9ZoQVkKVdquVavcZZJECNOGNfc7ywYOJO0EZNUXq/+T3/Lq+LQq2eT jGQttbL/dAXPrxUL8phgHzIv1kA4axvuSB20cFZgwE7yxJhpgLm7hIi3giLGicdZP7Gq /AFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=GQbBNeutQTO6ecrZYoPUyAu0VL/YjbG5ADPdI1TCfVg=; b=kC5BHdyvXsbUH8NKPZSuWRjeMMUHpIQ+mljSPPG/Nxo87zcHN6T9yktd/sxyxKHxC+ L8qaMqHqUjMCbexlcZs3n6JgHk1nG1zQLFqBaH4bEwk0dXu8kX2O99yhHg+zE2G0RXAt nHYSwHPusK+WBoD4yyHkagtXouM1w5VAO4a4BZXXXhvKfrEqq/oGpX64Iantg8DaVGkW jhURP+rzdDC5QFdGpMw+UdYPDeh2DOAqYaRbKa6JnungXLyGsUc7Sp3VN+sui9rmaXrs he5iCguam8h1gXy4WfWdGhk8kFfSw9vHwp09GXA9Rjo+ezuiMt9YSUwR8h2nfVNF8ZGv N1vQ== X-Gm-Message-State: AJIora+ec9cYSVbtF3uozJYkcZQgdB89GvWrZxCmIz8EXOkSUnBun0H5 JZ2FtX1cpUmkkFU8Opi31s8= X-Google-Smtp-Source: AGRyM1t43S3vDktiJ/z1cW6HdvFbtaEOGXHaFlHayIYyxlVLDi2fi1mplVPVY0gRZJ1pD7oCGFKxLA== X-Received: by 2002:a2e:22c6:0:b0:258:fd28:b253 with SMTP id i189-20020a2e22c6000000b00258fd28b253mr7022971lji.418.1655557150597; Sat, 18 Jun 2022 05:59:10 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id y14-20020a2eb00e000000b0025550801586sm923132ljk.132.2022.06.18.05.59.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 18 Jun 2022 05:59:09 -0700 (PDT) Date: Sat, 18 Jun 2022 15:59:08 +0300 From: Dmitry Kozlyuk To: Tyler Retzlaff Cc: dev@dpdk.org, thomas@monjalon.net, anatoly.burakov@intel.com, Narcisa Vasile Subject: Re: [PATCH v2 2/6] eal: add thread lifetime management Message-ID: <20220618155908.70e822af@sovereign> In-Reply-To: <1655250438-18044-3-git-send-email-roretzla@linux.microsoft.com> References: <1654783134-13303-1-git-send-email-roretzla@linux.microsoft.com> <1655250438-18044-1-git-send-email-roretzla@linux.microsoft.com> <1655250438-18044-3-git-send-email-roretzla@linux.microsoft.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 2022-06-14 16:47 (UTC-0700), Tyler Retzlaff: > On Windows, the function executed by a thread when the thread starts is > represeneted by a function pointer of type DWORD (*func) (void*). > On other platforms, the function pointer is a void* (*func) (void*). > > Performing a cast between these two types of function pointers to > uniformize the API on all platforms may result in undefined behavior. > TO fix this issue, a wrapper that respects the signature required by > CreateThread() has been created on Windows. The interface issue is still there: `rte_thread_func` allows the thread routine to have a pointer-sized result. `rte_thread_join()` allows to obtain this value as `unsigned long`, which is pointer-sized on 32-bit platforms and less than that on 64-bit platforms. This can lead to issues when developers assume they can return a pointer and this works on 32 bits, but doesn't work on 64 bits. If you want to keep API as close to phtread as possible, the limitation must be at least clearly documented in Doxygen (`rte_thread_func` is undocumented BTW). I also suggest using `uint32_t` instead of `unsigned long` precisely to avoid "is it pointer-sized?" doubts. (I don't see much benefit in keeping pthread-like signature. When moving from pthread to rte_thread, it is as trivial to change the thread routine return type.) > +int > +rte_thread_create(rte_thread_t *thread_id, > + const rte_thread_attr_t *thread_attr, > + rte_thread_func thread_func, void *args) > +{ > [...] > + 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; thread_map_priority_to_os_value() already checks for unsupported values, why not let it do this particular check? > +int > +rte_thread_join(rte_thread_t thread_id, unsigned long *value_ptr) > +{ > + int ret = 0; > + void *res = NULL; > + void **pres = NULL; > + > + if (value_ptr != NULL) > + pres = &res; > + > + ret = pthread_join((pthread_t)thread_id.opaque_id, pres); > + if (ret != 0) { > + RTE_LOG(DEBUG, EAL, "pthread_join failed\n"); > + return ret; > + } > + > + if (value_ptr != NULL && *pres != NULL) > + *value_ptr = *(unsigned long *)(*pres); > + > + return 0; > +} What makes *pres == NULL special? > +static DWORD > +thread_func_wrapper(void *args) > +{ > + struct thread_routine_ctx *pctx = args; > + struct thread_routine_ctx ctx; > + > + ctx.thread_func = pctx->thread_func; > + ctx.routine_args = pctx->routine_args; ctx = *pctx? > + > + free(pctx); > + > + return (DWORD)(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; > + DWORD tid; > + HANDLE thread_handle = NULL; > + GROUP_AFFINITY thread_affinity; > + struct thread_routine_ctx *ctx = NULL; > + > + 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; > + > + thread_handle = CreateThread(NULL, 0, thread_func_wrapper, ctx, > + CREATE_SUSPENDED, &tid); > + if (thread_handle == NULL) { > + ret = thread_log_last_error("CreateThread()"); > + free(ctx); > + goto cleanup; Missing `free(ctx)` from other error paths below. > + } > + thread_id->opaque_id = tid; > + > + if (thread_attr != NULL) { > + if (CPU_COUNT(&thread_attr->cpuset) > 0) { > + ret = convert_cpuset_to_affinity( > + &thread_attr->cpuset, > + &thread_affinity > + ); > + if (ret != 0) { > + RTE_LOG(DEBUG, EAL, "Unable to convert cpuset to thread affinity\n"); > + goto cleanup; > + } > + > + if (!SetThreadGroupAffinity(thread_handle, > + &thread_affinity, NULL)) { > + ret = thread_log_last_error("SetThreadGroupAffinity()"); > + goto cleanup; > + } > + } > + ret = rte_thread_set_priority(*thread_id, > + thread_attr->priority); > + if (ret != 0) { > + RTE_LOG(DEBUG, EAL, "Unable to set thread priority\n"); > + goto cleanup; > + } > + } > + > + if (ResumeThread(thread_handle) == (DWORD)-1) { > + ret = thread_log_last_error("ResumeThread()"); > + goto cleanup; > + } > + > +cleanup: > + if (thread_handle != NULL) { > + CloseHandle(thread_handle); > + thread_handle = NULL; > + } > + > + return ret; > +}