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 35B6CA0545; Mon, 20 Jun 2022 19:39:18 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C7E0E4069C; Mon, 20 Jun 2022 19:39:17 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id C4D5240151 for ; Mon, 20 Jun 2022 19:39:15 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id EA08D20C3669; Mon, 20 Jun 2022 10:39:14 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com EA08D20C3669 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1655746754; bh=3bS+wCvoeYYkOb5IIzOYxfkEpNK31bx2eFxOvqzx0q4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DCXENrKbwGoaneNgoLJkD3esYDnOrTiXXvqdwMv+4w7SJ2k400/QSENjDwUZjkKly CfpkU5bmQrhvdWUgCW1taic7WZW96Ftf+t5LVQRPJKNRoBfU6oRyVczx+SicSD9IfX Tw1gWDsP0UjTB3GT8fmqXBOMZqh4UaKEG/t52Mw4= Date: Mon, 20 Jun 2022 10:39:14 -0700 From: Tyler Retzlaff To: Dmitry Kozlyuk 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: <20220620173914.GA14239@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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> <20220618155908.70e822af@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220618155908.70e822af@sovereign> 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 Sat, Jun 18, 2022 at 03:59:08PM +0300, Dmitry Kozlyuk wrote: > 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.) thanks Dmitry, i wasn't aware this feedback had been given previously. let me digest the problem and we'll figure out how we can address it. > > > +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? hm, i hadn't noticed that was duplicated. let me try to eliminate it. > > > +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? ack > > > + > > + 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. ack > > > + } > > + 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; > > +} thanks let me tidy those bits up, i'll submit a revision this week.