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 B991DA034C; Wed, 22 Jun 2022 20:21:23 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 93BB240A84; Wed, 22 Jun 2022 20:21:23 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 1207C4069F for ; Wed, 22 Jun 2022 20:21:22 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 3379220C5A6F; Wed, 22 Jun 2022 11:21:21 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3379220C5A6F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1655922081; bh=an0YW8PjGshlEoMYQTINyIgo2ZmS87WfOsU8Wt3zKQU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=G7ddNNf6uXYlC1tTL/uZnG1wDtvAIKjd3oHd/NUwfNRd1E5y6OWPfnjTH/p+NMDbE ixVxXo245Tp0zDctY56m0++bhulVh0FNkhgO7HyYHadfmVOJEMO3triimbs/s4zZ7X P/7CiqAQwqY2WaXtnJXGZIWesJGnFx6LW0MIuu0A= Date: Wed, 22 Jun 2022 11:21:21 -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: <20220622182121.GA20387@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> <20220621185103.GC18214@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20220621224421.244a772f@sovereign> <20220621212823.GF18214@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20220622012414.09abc32a@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220622012414.09abc32a@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 Wed, Jun 22, 2022 at 01:24:14AM +0300, Dmitry Kozlyuk wrote: > 2022-06-21 14:28 (UTC-0700), Tyler Retzlaff: > > On Tue, Jun 21, 2022 at 10:44:21PM +0300, Dmitry Kozlyuk wrote: > > > 2022-06-21 11:51 (UTC-0700), Tyler Retzlaff: > > > > > > +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? > > > > > > > > it's not clear what you mean, can you explain? maybe there is some > > > > context i am missing from the original patch series? > > > > > > There's no previous context. > > > After ptread_join(), *pres holds the return value of the thread routine. > > > You only assign *value_ptr if value_ptr is not NULL (obviously correct) > > > and if *pres != NULL, that is, if the thread returned a non-NULL value. > > > But this value is opaque, why do you filter NULL? > > > > i don't think it is opaque here? unsigned long * value_ptr says we have > > to store an integer. which leads to a discussion of what should get > > stored at the value_ptr location if pthread_join() itself returns no > > result but the caller of rte_thread_join() requests the result. > > There is no question. If `pthread_join()` fails, the function exits early > and `*value_ptr` remains unmodified. If `pthread_join()` succeeds > with a non-NULL second argument (`pres`), `*pres` aka `res` is always filled. > NULL can be placed there too if that's what the thread routine has returned. okay, discussed offline it was just my misunderstanding of impact on the conditional block in the presence of the NULL check. as discussed we'll rewrite the check as. if (value_ptr != NULL) *value_ptr = (uint32_t)(uintptr_t)res; this doesn't double dereference pres as the original code was and therefore the *pres NULL check is unnecessary. thanks!