From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 48EC01B64C for ; Wed, 31 Jan 2018 17:45:21 +0100 (CET) Received: from lfbn-lil-1-110-231.w90-45.abo.wanadoo.fr ([90.45.197.231] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1egvVq-0006VZ-8n; Wed, 31 Jan 2018 17:45:31 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Wed, 31 Jan 2018 17:45:19 +0100 Date: Wed, 31 Jan 2018 17:45:19 +0100 From: Olivier Matz To: Andrew Rybchenko Cc: dev@dpdk.org Message-ID: <20180131164519.b3kohaud3mgs4rqp@platinum> References: <1511539591-20966-1-git-send-email-arybchenko@solarflare.com> <1516713372-10572-1-git-send-email-arybchenko@solarflare.com> <1516713372-10572-3-git-send-email-arybchenko@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1516713372-10572-3-git-send-email-arybchenko@solarflare.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [RFC v2 02/17] mempool: add op to calculate memory size to be allocated X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Jan 2018 16:45:21 -0000 On Tue, Jan 23, 2018 at 01:15:57PM +0000, Andrew Rybchenko wrote: > Size of memory chunk required to populate mempool objects depends > on how objects are stored in the memory. Different mempool drivers > may have different requirements and a new operation allows to > calculate memory size in accordance with driver requirements and > advertise requirements on minimum memory chunk size and alignment > in a generic way. > > Suggested-by: Olivier Matz > Signed-off-by: Andrew Rybchenko The general idea is fine. Few small comments below. [...] > --- > lib/librte_mempool/rte_mempool.c | 95 ++++++++++++++++++++++-------- > lib/librte_mempool/rte_mempool.h | 63 +++++++++++++++++++- > lib/librte_mempool/rte_mempool_ops.c | 18 ++++++ > lib/librte_mempool/rte_mempool_version.map | 8 +++ > 4 files changed, 159 insertions(+), 25 deletions(-) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index e783b9a..1f54f95 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -233,13 +233,14 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags, > return sz->total_size; > } > > - > /* > - * Calculate maximum amount of memory required to store given number of objects. > + * Internal function to calculate required memory chunk size shared > + * by default implementation of the corresponding callback and > + * deprecated external function. > */ > -size_t > -rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift, > - unsigned int flags) > +static size_t > +rte_mempool_xmem_size_int(uint32_t elt_num, size_t total_elt_sz, > + uint32_t pg_shift, unsigned int flags) > { I'm not getting why the function is changed to a static function in this patch, given that rte_mempool_xmem_size() is redefined below as a simple wrapper. > size_t obj_per_page, pg_num, pg_sz; > unsigned int mask; > @@ -264,6 +265,49 @@ rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift, > return pg_num << pg_shift; > } > > +ssize_t > +rte_mempool_calc_mem_size_def(const struct rte_mempool *mp, > + uint32_t obj_num, uint32_t pg_shift, > + size_t *min_chunk_size, > + __rte_unused size_t *align) > +{ > + unsigned int mp_flags; > + int ret; > + size_t total_elt_sz; > + size_t mem_size; > + > + /* Get mempool capabilities */ > + mp_flags = 0; > + ret = rte_mempool_ops_get_capabilities(mp, &mp_flags); > + if ((ret < 0) && (ret != -ENOTSUP)) > + return ret; > + > + total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size; > + > + mem_size = rte_mempool_xmem_size_int(obj_num, total_elt_sz, pg_shift, > + mp->flags | mp_flags); > + > + if (mp_flags & MEMPOOL_F_CAPA_PHYS_CONTIG) > + *min_chunk_size = mem_size; > + else > + *min_chunk_size = RTE_MAX((size_t)1 << pg_shift, total_elt_sz); > + > + /* No extra align requirements by default */ maybe set *align = 0 ? I think it's not sane to keep the variable uninitialized. [...] > +/** > + * Calculate memory size required to store specified number of objects. > + * > + * Note that if object size is bigger then page size, then it assumes > + * that pages are grouped in subsets of physically continuous pages big > + * enough to store at least one object. > + * > + * @param mp > + * Pointer to the memory pool. > + * @param obj_num > + * Number of objects. > + * @param pg_shift > + * LOG2 of the physical pages size. If set to 0, ignore page boundaries. > + * @param min_chunk_size > + * Location for minimum size of the memory chunk which may be used to > + * store memory pool objects. > + * @param align > + * Location with required memory chunk alignment. > + * @return > + * Required memory size aligned at page boundary. > + */ > +typedef ssize_t (*rte_mempool_calc_mem_size_t)(const struct rte_mempool *mp, > + uint32_t obj_num, uint32_t pg_shift, > + size_t *min_chunk_size, size_t *align); The API comment can be enhanced by saying that min_chunk_size and align are output only parameters. For align, the '0' value could be described as well. > + > +/** > + * Default way to calculate memory size required to store specified > + * number of objects. > + */ > +ssize_t rte_mempool_calc_mem_size_def(const struct rte_mempool *mp, > + uint32_t obj_num, uint32_t pg_shift, > + size_t *min_chunk_size, size_t *align); > + The behavior of the default function could be better explained. I would prefer "default" instead of "def".