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 88EF542C4C; Wed, 7 Jun 2023 10:33:53 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6192B40A84; Wed, 7 Jun 2023 10:33:53 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id E51A340698 for ; Wed, 7 Jun 2023 10:33:51 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id B03A420266; Wed, 7 Jun 2023 10:33:51 +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: [PATCH] mempool: optimize get objects with constant n X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Wed, 7 Jun 2023 10:33:49 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87996@smartserver.smartshare.dk> In-Reply-To: <3590085.WbyNdk4fJJ@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH] mempool: optimize get objects with constant n Thread-Index: AdmZF4Mmm/sljaNlTSGTkV2x9l8aSQAAIuxQ References: <20230411064845.37713-1-mb@smartsharesystems.com> <1739067.KUTt5R2Mg1@thomas> <98CBD80474FA8B44BF855DF32C47DC35D87995@smartserver.smartshare.dk> <3590085.WbyNdk4fJJ@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" Cc: "Bruce Richardson" , , , 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Wednesday, 7 June 2023 10.10 >=20 > 07/06/2023 10:03, Morten Br=F8rup: > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > Sent: Wednesday, 7 June 2023 09.52 > > > > > > 18/04/2023 14:55, Bruce Richardson: > > > > On Tue, Apr 18, 2023 at 01:29:49PM +0200, Morten Br=F8rup wrote: > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > > > > > On Tue, Apr 11, 2023 at 08:48:45AM +0200, Morten Br=F8rup = wrote: > > > > > > > + if (__extension__(__builtin_constant_p(n)) && n <=3D = cache->len) { > > > > > > > + /* > > > > > > > + * The request size is known at build time, and > > > > > > > + * the entire request can be satisfied from the cache, > > > > > > > + * so let the compiler unroll the fixed length copy = loop. > > > > > > > + */ > > > > > > > + cache->len -=3D n; > > > > > > > + for (index =3D 0; index < n; index++) > > > > > > > + *obj_table++ =3D *--cache_objs; > > > > > > > + > > > > > > > > > > > > This loop looks a little awkward to me. Would it be clearer = (and > perhaps > > > > > > easier for compilers to unroll efficiently if it was = rewritten as: > > > > > > > > > > > > cache->len -=3D n; > > > > > > cache_objs =3D &cache->objs[cache->len]; > > > > > > for (index =3D 0; index < n; index++) > > > > > > obj_table[index] =3D cache_objs[index]; > > > > > > > > > > The mempool cache is a stack, so the copy loop needs get the = objects > in > > > decrementing order. I.e. the source index decrements and the = destination > index > > > increments. > > > > > > > > > > > > > BTW: Please add this as a comment in the code too, above the = loop to > avoid > > > > future developers (or even future me), asking this question = again! > > > > > > Looks like this request was missed. > > > > I intentionally omitted it, because I disagree with the suggestion. > > > > Otherwise, reminders that the mempool cache is a stack should be = plastered > all over the source code, not just here. For reference, this copy loop > (without such a reminder) also exists elsewhere in the same file. > > > > Apologies for not responding to Bruce's request earlier. >=20 > What about doing a general comment at the top of the function, > with the assignment of the pointer at the end of the array: >=20 > /* The cache is a stack, so copy will be in reverse order. */ > cache_objs =3D &cache->objs[cache->len]; >=20 > I could do it on apply if there is an agreement. >=20 ACK from me. For consistency, please also add the reminder to the two existing = reverse order copy loops in rte_mempool_do_generic_get().