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 BF336A0548; Wed, 16 Jun 2021 17:01:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4AF754067A; Wed, 16 Jun 2021 17:01:50 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id BCD6540140 for ; Wed, 16 Jun 2021 17:01:49 +0200 (CEST) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Wed, 16 Jun 2021 17:01:46 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C61864@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [dpdk-dev] [PATCH] parray: introduce internal API for dynamic arrays Thread-Index: Addir/K+RRlwwSyjTMG7DYmdVRu+QAACn1cg References: <20210614105839.3379790-1-thomas@monjalon.net> <2004320.XGyPsaEoyj@thomas> <98CBD80474FA8B44BF855DF32C47DC35C61851@smartserver.smartshare.dk> <1857954.7Ex43hCf9S@thomas> <98CBD80474FA8B44BF855DF32C47DC35C61862@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" Cc: "Jerin Jacob" , "Thomas Monjalon" , "dpdk-dev" , "Olivier Matz" , "Andrew Rybchenko" , "Honnappa Nagarahalli" , "Ananyev, Konstantin" , "Ferruh Yigit" , "Jerin Jacob" , "Akhil Goyal" 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" > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson > Sent: Wednesday, 16 June 2021 15.03 >=20 > On Wed, Jun 16, 2021 at 01:27:17PM +0200, Morten Br=F8rup wrote: > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com] > > > Sent: Wednesday, 16 June 2021 11.42 > > > > > > On Tue, Jun 15, 2021 at 12:18 PM Thomas Monjalon > > > > wrote: > > > > > > > > 14/06/2021 17:48, Morten Br=F8rup: > > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas > > > Monjalon > > > > > It would be much simpler to just increase RTE_MAX_ETHPORTS to > > > something big enough to hold a sufficiently large array. And > possibly > > > add an rte_max_ethports variable to indicate the number of > populated > > > entries in the array, for use when iterating over the array. > > > > > > > > > > Can we come up with another example than RTE_MAX_ETHPORTS = where > > > this library provides a better benefit? > > > > > > > > What is big enough? > > > > Is 640KB enough for RAM? ;) > > > > > > If I understand it correctly, Linux process allocates 640KB due to > > > that fact currently > > > struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS] is global and > it > > > is from BSS. > > > > Correct. > > > > > If we make this from heap i.e use malloc() to allocate this memory > > > then in my understanding Linux > > > really won't allocate the real page for backend memory until > unless, > > > someone write/read to this memory. > > > > If the array is allocated from the heap, its members will be = accessed > though a pointer to the array, e.g. in rte_eth_rx/tx_burst(). This > might affect performance, which is probably why the array is allocated > the way it is. > > >=20 > It depends on whether the array contains pointers to malloced elements > or > the array itself is just a single malloced array of all the = structures. > While I think the parray proposal referred to the former - which would > have > an extra level of indirection - the switch we are discussing here is > the > latter which should have no performance difference, since the method = of > accessing the elements will be the same, only with the base address > pointing to a different area of memory. I was not talking about an array of pointers. And it is not the same: int arr[27]; int * parr =3D arr; // direct access int dir(int i) { return arr[i]; } // indirect access int indir(int i) { return parr[i]; } The direct access knows the address of arr, so it will compile to: movsx rdi, edi mov eax, DWORD PTR arr[0+rdi*4] ret The indirect access needs to first read the memory location holding the = pointer to the array, and then it can read the array member, so it will = compile to: mov rax, QWORD PTR parr[rip] movsx rdi, edi mov eax, DWORD PTR [rax+rdi*4] ret >=20 > > Although it might be worth investigating how much it actually = affects > the performance. > > > > So we need to do something else if we want to conserve memory and > still allow a large rte_eth_devices[] array. > > > > Looking at struct rte_eth_dev, we could reduce its size as follows: > > > > 1. Change the two callback arrays > post_rx/pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT] to pointers to > callback arrays, which are allocated from the heap. > > With the default RTE_MAX_QUEUES_PER_PORT of 1024, these two arrays > are the sinners that make the struct rte_eth_dev use so much memory. > This modification would save 16 KB (minus 16 bytes for the pointers to > the two arrays) per port. > > Furthermore, these callback arrays would only need to be allocated = if > the application is compiled with callbacks enabled (#define > RTE_ETHDEV_RXTX_CALLBACKS). And they would only need to be sized to = the > actual number of queues for the port. > > > > The disadvantage is that this would add another level of = indirection, > although only for applications compiled with callbacks enabled. > > > This seems reasonable to at least investigate. >=20 > > 2. Remove reserved_64s[4] and reserved_ptrs[4]. This would save 64 > bytes per port. Not much, but worth considering if we are changing the > API/ABI anyway. > > > I strongly dislike reserved fields to I would tend to favour these. > However, it does possibly reduce future compatibility if we do need to > add > something to ethdev. There should be an official policy about adding reserved fields for = future compatibility. I'm against adding them, unless it can be argued = that they are likely to match what is needed in the future; in the real = world there is no way to know if they match future requirements. >=20 > Another option is to split ethdev into fast-path and non-fastpath = parts > - > similar to Konstantin's suggestion of just having an array of the ops. > We > can have an array of minimal structures with fastpath ops and queue > pointers, for example, with an ethdev-private pointer to the rest of > the > struct elsewhere in memory. Since that second struct would be = allocated > on-demand, the size of the ethdev array can be scaled with far smaller > footprint. >=20 > /Bruce The rte_eth_dev structures are really well organized now. E.g. the rx/tx = function pointers and the pointer to the shared memory data of the = driver are in the same cache line. We must be very careful if we change = them. Also, rte_ethdev.h and rte_ethdev_core.h are easy to read and = understand. -Morten