From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 628C61B8EB for ; Thu, 1 Feb 2018 08:15:57 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id DF9C3980067; Thu, 1 Feb 2018 07:15:55 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Thu, 1 Feb 2018 07:15:51 +0000 To: Olivier Matz CC: 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> <20180131164519.b3kohaud3mgs4rqp@platinum> From: Andrew Rybchenko Message-ID: <00e70bd6-6409-da85-95b2-ad039bd1a4a7@solarflare.com> Date: Thu, 1 Feb 2018 10:15:47 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180131164519.b3kohaud3mgs4rqp@platinum> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23634.003 X-TM-AS-Result: No--18.945700-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1517469356-Xekfc8Qfp0Lf Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: Thu, 01 Feb 2018 07:15:57 -0000 On 01/31/2018 07:45 PM, Olivier Matz wrote: > On Tue, Jan 23, 2018 at 01:15:57PM +0000, Andrew Rybchenko wrote: >> 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. rte_mempool_xmem_size() is deprecated in the subsequent patch. This static function is created to reuse the code and avoid usage of the deprecated in non-deprecated. Yes, it is definitely unclear here and now I think it is better to make the change in the patch which deprecates rte_mempool_xmem_size(). >> 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. Right now align is in/out. On input it is either cacheline (has hugepages) or page size. These external limitations could be important for size calculations. _ops_calc_mem_size() is allowed to strengthen the alignment only. If it is acceptable, I'll try to highlight it in the description and check it in the code. May be more transparent solution is to make it input-only and highlight that it is full responsibility of the callback to care about all alignment requirements (e.g. imposed by absence of huge pages). What do you think? > [...] > >> +/** >> + * 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. OK, will fix as soon as we decide if align is input only or input/output. >> + >> +/** >> + * 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". Will do.