From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Wed, 22 Jun 2022 00:24:16 +0200 (CEST)
Received: by mail-lf1-f53.google.com with SMTP id t24so12466666lfr.4
for <dev@dpdk.org>; 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 <dmitry.kozliuk@gmail.com>
To: Tyler Retzlaff <roretzla@linux.microsoft.com>
Cc: dev@dpdk.org, thomas@monjalon.net, anatoly.burakov@intel.com, Narcisa
Vasile <navasile@microsoft.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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.