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 60E42430BE; Mon, 21 Aug 2023 11:36:16 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4680B427E9; Mon, 21 Aug 2023 11:36:16 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 108B040A7D for ; Mon, 21 Aug 2023 11:36:15 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id D036B205ED; Mon, 21 Aug 2023 11:36:14 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: Bug in non-power-of-2 rings? Date: Mon, 21 Aug 2023 11:36:13 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87B23@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: Bug in non-power-of-2 rings? Thread-Index: AdnTRcFJLofpPgUJQi2nwmJrDMr1dgAtIR+AAAVbEBAAAJqDwA== References: <98CBD80474FA8B44BF855DF32C47DC35D87B20@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Konstantin Ananyev" , "Bruce Richardson" Cc: , , , , 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 +CC Olivier, referring to your review of the original patch. > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com] > Sent: Monday, 21 August 2023 11.29 >=20 > Hi everyone, >=20 > > On Sun, Aug 20, 2023 at 11:07:33AM +0200, Morten Br=F8rup wrote: > > > Bruce, Honnappa, Konstantin, > > > > > > Back in 2017, Bruce added support for non-power-of-2 rings with = this > patch [1]. > > > > > > [1]: > = https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring.h?id=3Db7446115= 5 > 543430f5253e96ad6d413ebcad36693 > > > > > > I think that the calculation of "entries" in > __rte_ring_move_cons_head() [2][3] is incorrect when the ring capacity > is not power-of-2, > > because it is missing the capacity comparison you added to > rte_ring_count() [4]. Please review if I'm mistaken. > > > > > > [2]: > = https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_c11_pvt. > h#L159 > > > [3]: > = https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_generic_ > pvt.h#L150 > > > [4]: > https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring.h#L502 >=20 > Just to confirm you suggest something like that: > - *entries =3D (r->prod.tail - *old_head); > + count =3D (r->prod.tail - *old_head); > + entries =3D (count > r->capacity) ? r->capacity : count; > right? Yes, since rte_ring_count() does it, it might be required here too. >=20 > > > > > thanks for flagging this inconsistency, but I think we are ok. > > > > For consumer, I think this is correct, because we are only ever > reducing > > the number of entries in the ring, and the calculation of the number > of > > entries is made in the usual way using modulo arithmetic. We should > never > > have more than capacity entries in the ring so the check in ring = count > I > > believe is superflous. [The exception would be if someone bypassed = the > > inline functions and accessed the ring directly themselves - at = which > point > > "all bets are off", to use the English phrase] I have now found the comments to the original patch [5]. It seems that = Olivier flagged this as a risk for rte_ring_free_count(), which reads = r->prod.tail and r->cons.tail without synchronization. However, since __rte_ring_move_cons_head() uses synchronization, I guess = that such a risk is not present here. [5]: = https://patchwork.dpdk.org/project/dpdk/patch/20170607133620.275801-2-bru= ce.richardson@intel.com/ > > > > The producer code (__rte_ring_move_prod_head) does do a capacity > check, > > which is where one is required to ensure we never exceed capacity. Agreed. Thanks for double checking. >=20 > I also can't come up with the case, when current code will cause an > issue.. > In properly operating ring, I think we should never have more then r- > >capacity > entries populated, so this extra check can be skipped. > Unless you do have some particular case in mind? No special case in mind. Just stumbled over this code looking different = than similar code in rte_ring_free_count().