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 74335A0C44; Tue, 15 Jun 2021 16:03:06 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F16D44067A; Tue, 15 Jun 2021 16:03:05 +0200 (CEST) Received: from new3-smtp.messagingengine.com (new3-smtp.messagingengine.com [66.111.4.229]) by mails.dpdk.org (Postfix) with ESMTP id EA12240140 for ; Tue, 15 Jun 2021 16:03:03 +0200 (CEST) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.nyi.internal (Postfix) with ESMTP id 7C1AA580960; Tue, 15 Jun 2021 10:03:01 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Tue, 15 Jun 2021 10:03:01 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm1; bh= FMFo1p8QLPJrbx5Ysr9en7AcJAJTQaUmf7xbrK9Sjig=; b=Iql1bwQ77EuMb2Kv 4hc7sgcLiH32/COq7/WhGGkwRGmq7JQHvFM/RAGiWgXow63twmm8X/wRT4uP2uqy PEifK9mkK4oeq+O2Yqxf3+8/bvbSfi5g/Wcsgo7JtHQlRUaYEkx4W7PyIXsXdbg8 BM4QQ7EcE6LqRpA6mBuOa6MPNLx75+QMVELlNGQGvjmkY0wYTWEIe2hXlGWv2i2h BBAP7Od2Lbzq+0yQUsNzG4uduT0ZxZIq7Sgx4pR+t+1SeAelplkAdXCgzWW920QT p9wo0Mio73w7fAS/ENPBFng8Y+1xiKgIOpyncw15hxG29Dzxft1+uXe6jiYKfd07 ENpRZg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=FMFo1p8QLPJrbx5Ysr9en7AcJAJTQaUmf7xbrK9Sj ig=; b=TFDAz1Hq4MhH8+gvKjJ7BRLOxHj/UfIGi1OFKfdCqwba/+F1rHygCBsx+ miPsYzs2G+lC/eYOUPFDYt3GKDRQj7ydgXi2Da26RrGo3SSYBfnQc7wjuNaDdzgl 7uodNJfnVRnufJ6qyjb/STYPYVZ4mOydvZFYelSQA4wA5B09QyPNBy5Ju/p0awqs UhHBiXzcTBUAaDMgdZ+8bTVDwPR5fhigGlrgB4VVPNOA6f+skWh+ixFoo/Z2FI6r yxCC/vMQ5+My5Y/ve6fApf2l6XQRkmoJLr5DAx33RXQp7PTqkcOA0RRhhA0rgLQh 0WAFfrgVA+vvdPXub0tIW1qYI6cow== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfedvjedgjedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtqhertddttdejnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepkeethedtieevhfeigeejleegudefjeehkeekteeuveeiuedvveeu tdejveehveetnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 15 Jun 2021 10:02:59 -0400 (EDT) From: Thomas Monjalon To: Jerin Jacob , "Ananyev, Konstantin" Cc: "Richardson, Bruce" , Morten =?ISO-8859-1?Q?Br=F8rup?= , "dev@dpdk.org" , "olivier.matz@6wind.com" , "andrew.rybchenko@oktetlabs.ru" , "honnappa.nagarahalli@arm.com" , "Yigit, Ferruh" , "jerinj@marvell.com" , "gakhil@marvell.com" Date: Tue, 15 Jun 2021 16:02:58 +0200 Message-ID: <2133178.xeBKh9gUzh@thomas> In-Reply-To: References: <20210614105839.3379790-1-thomas@monjalon.net> <27997952.XWs9bGgn2z@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH] parray: introduce internal API for dynamic arrays 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 Sender: "dev" 15/06/2021 12:08, Ananyev, Konstantin: > > 15/06/2021 11:33, Ananyev, Konstantin: > > > > 14/06/2021 17:48, Jerin Jacob: > > > > > On Mon, Jun 14, 2021 at 8:29 PM Ananyev, Konstantin > > > > > wrote: > > > > > > I had only a quick look at your approach so far. > > > > > > But from what I can read, in MT environment your suggestion wil= l require > > > > > > extra synchronization for each read-write access to such parray= element (lock, rcu, ...). > > > > > > I think what Bruce suggests will be much ligther, easier to imp= lement and less error prone. > > > > > > At least for rte_ethdevs[] and friends. > > > > > > > > > > +1 > > > > > > > > Please could you have a deeper look and tell me why we need more lo= cks? > > > > The element pointers doesn't change. > > > > Only the array pointer change at resize, > > > > > > Yes, array pointer changes at resize, and reader has to read that val= ue > > > to access elements in the parray. Which means that we need some sync > > > between readers and updaters to avoid reader using stale pointer (ref= =2Dcounter, rcu, etc.). > >=20 > > No > > The old array is still there, so we don't need sync. > >=20 > > > I.E. updater can free old array pointer *only* when it can guarantee = that there are no > > > readers that still use it. > >=20 > > No > > Reading an element is OK because the pointer to the element is not chan= ged. > > Getting the pointer to an element from the index is the only thing > > which is blocking the freeing of an array, > > and I see no reason why dereferencing an index would be longer > > than 2 consecutive resizes of the array. >=20 > In general, your thread can be switched off the cpu at any moment. > And you don't know for sure when it will be scheduled back. >=20 > >=20 > > > > but the old one is still usable until the next resize. > > > > > > Ok, but what is the guarantee that reader would *always* finish till = next resize? > > > As an example of such race condition: > > > > > > /* global one */ > > > struct rte_parray pa; > > > > > > /* thread #1, tries to read elem from the array */ > > > .... > > > int **x =3D pa->array; > >=20 > > We should not save the array pointer. > > Each index must be dereferenced with the macro > > getting the current array pointer. > > So the interrupt is during dereference of a single index. >=20 > You still need to read your pa->array somewhere (let say into a register). > Straight after that your thread can be interrupted. > Then when it is scheduled back to the CPU that value (in a register) migh= t be s stale one. >=20 > >=20 > > > /* thread # 1 get suspended for a while at that point */ > > > > > > /* meanwhile thread #2 does: */ > > > .... > > > /* causes first resize(), x still valid, points to pa->old_array */ > > > rte_parray_alloc(&pa, ...); > > > ..... > > > /* causes second resize(), x now points to freed memory */ > > > rte_parray_alloc(&pa, ...); > > > ... > >=20 > > 2 resizes is a very long time, it is at minimum 33 allocations! > >=20 > > > /* at that point thread #1 resumes: */ > > > > > > /* contents of x[0] are undefined, 'p' could point anywhere, > > > might cause segfault or silent memory corruption */ > > > int *p =3D x[0]; > > > > > > > > > Yes probability of such situation is quite small. > > > But it is still possible. > >=20 > > In device probing, I don't see how it is realistically possible: > > 33 device allocations during 1 device index being dereferenced. >=20 > Yeh, it would work fine 1M times, but sometimes will crash. Sometimes a thread will be interrupted during 33 device allocations? > Which will make it even harder to reproduce, debug and fix. > I think that when introducing a new generic library into DPDK, > we should avoid making such assumptions. I intend to make it internal-only (I should have named it eal_parray). > > I agree it is tricky, but that's the whole point of finding tricks > > to keep fast code. >=20 > It is not tricky, it is buggy =F0=9F=98=8A > You introducing a race condition into the new core generic library by des= ign, > and trying to convince people that it is *OK*. Yes, because I am convinced myself. > Sorry, but NACK from me till that issue will be addressed. It is not an issue, but a design. If you think that a thread can be interrupted during 33 device allocations then we should find another implementation, but I am quite sure it will be = slower.