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 DC386A00C3; Wed, 22 Jun 2022 00:24:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B44634069C; Wed, 22 Jun 2022 00:24:17 +0200 (CEST) Received: from mail-lf1-f53.google.com (mail-lf1-f53.google.com [209.85.167.53]) by mails.dpdk.org (Postfix) with ESMTP id D041340151 for ; Wed, 22 Jun 2022 00:24:16 +0200 (CEST) Received: by mail-lf1-f53.google.com with SMTP id t24so12466666lfr.4 for ; Tue, 21 Jun 2022 15:24:16 -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=U8cLpkMJkxE7WrPe8nfo0/vImf36lAjVx82Z6JASpRo=; b=GTbECYXh/fluUVdss13dWWrx8agk215tgdti+R6yZjh66yX0vcFa4FEIv0XdKrf26v FDm34SVL08o0PWfZPesgNGlXBT4C3cDQ28v8ZM6MmGRBJXCxZLKpTJvc2kwBokcWge+8 nL50tIvmqkb/L9+ZHaVJeHAY7NSTaY0ag58PtYHTHXWP2bWVmGSgf6nAj81S2gu/poMN qX5A0vH/bUyJv77WhfiR+gJMncW6jyU8mjtNLuiof9vHHwfnEsj5nkjMsTNSp7B7+ZLW RwTxgyoOSlNr4KPNKKfOHzFAFc78JY9k44f59gj4OFOjVSCNleWSYCKVnuk1YV4et8jI aI3g== 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=U8cLpkMJkxE7WrPe8nfo0/vImf36lAjVx82Z6JASpRo=; b=fdnk4tevNXkp0HUyn/ENqnnaFiN9MZG5GEGEv/1uT64CkHS4V1C3W20zn8nOXi5Y82 GfuEs/1UkmbNkderV3erxI2jA8yUlM8D4AQpAcF8bAjn2aTD40a4FqF7dttoQjWqYkBo N/srJ4GqF5iswHuzcFDy4KG9JfNmG5rdRSyUfiss0xFcQ9cwrctTNHJXL9y+670/hYKh arZsKMf1e4z8EUgSPgOT7Htt3xAmTEmBqUqGtX96o5OjUUFVpr15h/OrrUVBV+jzhCrp nyia8T7rdZonWb91u6W9pEL15brKHVNtC+e5Nd0RBi606CNl5NwPdnG/ZQP3QZ3PU3rC 6rwg== X-Gm-Message-State: AJIora/eRNlCFyteP1InGEgis9+4xTG+0fNm5OyM2ytTK3roVFTFLvn8 CfI7v93ZRAOmWiW89mGf+VM= X-Google-Smtp-Source: AGRyM1tYhsMjkbZuGtQyja9A5xQK8zaoC7qBgMWj2cO3/RnMMZGuFSLga4l9WSCTnPkD8fp8WfbXGw== X-Received: by 2002:a05:6512:3c84:b0:47f:7781:e49a with SMTP id h4-20020a0565123c8400b0047f7781e49amr271790lfv.370.1655850256220; Tue, 21 Jun 2022 15:24:16 -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 x6-20020a19e006000000b0047ac01fc644sm2321551lfg.44.2022.06.21.15.24.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Jun 2022 15:24:15 -0700 (PDT) Date: Wed, 22 Jun 2022 01:24:14 +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: <20220622012414.09abc32a@sovereign> In-Reply-To: <20220621212823.GF18214@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> 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-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. > > > Perhaps you meant if (pres != NULL), no dereference? > > that i think is just a repeat of a test checking if the caller of > rte_thread_join is interested in the result? > i.e. value_ptr != NULL -> pres != NULL > > both pres and *pres are dereferenced so it seems to track that prior to > those dereferences they have to be validated as being non-NULL. > i don't see how we could avoid dereferencing **pres to satisfy the > calling contract when the result is requested. > > now if value_ptr was unsigned long ** i guess i'd understand. i could > always be reading the code wrong. but thinking about further there is > another problem with this in that we really don't know what is being > aliased in *pres when using the pthread implementation, since pthread > could be returning a pointer to something narrow or with unknown layout > where later dereferencing it as something wider or in this case > specifically as unsigned long * would have horrible consequences. Sorry, no, it's just all wrong. We get a pointer from `pthread_join()` and store it in `res`, which can also be accessed as `*pres`, because `pres == &res`. Then we store the lower bits of `res` in an `*value_ptr`. We don't care what `res` points to because it is never dereferenced. > > i think this ends up semi-related to your other comment about what the > result type from rte_thread_func is, we can discuss offline and post > details back to the list.