From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id ECF50A0471 for ; Wed, 17 Jul 2019 15:37:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CFF701BD4F; Wed, 17 Jul 2019 15:37:50 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 4E5CA1B9ED for ; Wed, 17 Jul 2019 15:37:49 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 9A0C9980099; Wed, 17 Jul 2019 13:37:06 +0000 (UTC) Received: from [192.168.1.11] (85.187.13.152) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 17 Jul 2019 14:36:57 +0100 To: , CC: , , , , , References: <20190625035700.2953-1-vattunuru@marvell.com> <20190717090408.13717-1-vattunuru@marvell.com> <20190717090408.13717-2-vattunuru@marvell.com> From: Andrew Rybchenko Message-ID: Date: Wed, 17 Jul 2019 16:36:37 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190717090408.13717-2-vattunuru@marvell.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [85.187.13.152] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24768.000 X-TM-AS-Result: No-15.091600-8.000000-10 X-TMASE-MatchedRID: C/snMIRQLS3oSitJVour/fZvT2zYoYOwC/ExpXrHizy7Iv2OMTayQWtw OoXDbvJwuTZLG6KdNeBjhrNbizn3E1hCw3CsBjtuGUkpc56lLW3ZCLnoVnxYBovrg5+wrJbWa0I EkYOsokUToiOSoApYdGpSYPxhGlMNUs6pYzn13cswYApm54/SZsu99zcLpJbCdBaEtWosUzVyKM xmuvaJna3aC25avUuaJ/Q/cHhPAkofKML5AJtfLQ97mDMXdNW3HS44sLaHAPIed11F9IsKFA+Qc BVe4lZ5lpKpNiL4Llqi+hwIv0GEATlwBKEYzl7Th2VzUlo4HVOiIpNv3rjMdS4mS5Zcbgyks1Pp 95vbWarwOGawsB401YuUwurXGmwxMTzTu/0RGpV1e7Xbb6Im2o2QIlTs17VzT7zqZowzdpJQjev vFeK6vYqPZRyCYAfG6woT1Yg+BInEAPZCs5UQ+7bQFsbjObJefS0Ip2eEHny+qryzYw2E8LLn+0 Vm71Lcq7rFUcuGp/EgBwKKRHe+r6BD0+uQb8bOeZs5o4ZKejzkHJo6q0uOkBx3bPWQ1IJumH9xk 8wpOU0= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--15.091600-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24768.000 X-MDID: 1563370627-L_4zAtsleswB Subject: Re: [dpdk-dev] [PATCH v7 1/4] mempool: modify mempool populate() to skip objects from page boundaries 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 7/17/19 12:04 PM, vattunuru@marvell.com wrote: > From: Vamsi Attunuru > > Currently the phys address of a mempool object populated by the mempool > populate default() routine may not be contiguous with in that mbuf range. > > Patch ensures that each object's phys address is contiguous by modifying > default behaviour of mempool populate() to prevent objects from being > across 2 pages, expect if the size of object is bigger than size of page. > > Since the overhead after this modification will be very minimal considering > the hugepage sizes of 512M & 1G, default behaviour is modified except for > the object sizes bigger than the page size. > > Signed-off-by: Vamsi Attunuru > Signed-off-by: Kiran Kumar K NACK Looking at MEMPOOL_F_NO_IOVA_CONTIG description I don't understand why the patch is necessary at all. So, I'd like to know more. Exact conditions, IOVA mode, hugepage sizes, mempool flags and how it is populated. > --- > lib/librte_mempool/rte_mempool.c | 2 +- > lib/librte_mempool/rte_mempool_ops_default.c | 33 ++++++++++++++++++++++++++-- > 2 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index 7260ce0..1c48325 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -339,7 +339,7 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr, > i = rte_mempool_ops_populate(mp, mp->size - mp->populated_size, > (char *)vaddr + off, > (iova == RTE_BAD_IOVA) ? RTE_BAD_IOVA : (iova + off), > - len - off, mempool_add_elem, NULL); > + len - off, mempool_add_elem, opaque); The last argument is the callback opaque value. mempool_add_elem() does not use the opaque. But it is incorrect to use it for other purposes and require it to be memzone. > /* not enough room to store one object */ > if (i == 0) { > diff --git a/lib/librte_mempool/rte_mempool_ops_default.c b/lib/librte_mempool/rte_mempool_ops_default.c > index 4e2bfc8..85da264 100644 > --- a/lib/librte_mempool/rte_mempool_ops_default.c > +++ b/lib/librte_mempool/rte_mempool_ops_default.c > @@ -45,19 +45,48 @@ rte_mempool_op_calc_mem_size_default(const struct rte_mempool *mp, > return mem_size; > } > > +/* Returns -1 if object falls on a page boundary, else returns 0 */ > +static inline int > +mempool_check_obj_bounds(void *obj, uint64_t hugepage_sz, size_t elt_sz) > +{ > + uintptr_t page_end, elt_addr = (uintptr_t)obj; > + uint32_t pg_shift = rte_bsf32(hugepage_sz); > + uint64_t page_mask; > + > + page_mask = ~((1ull << pg_shift) - 1); > + page_end = (elt_addr & page_mask) + hugepage_sz; > + > + if (elt_addr + elt_sz > page_end) > + return -1; > + > + return 0; > +} > + > int > rte_mempool_op_populate_default(struct rte_mempool *mp, unsigned int max_objs, > void *vaddr, rte_iova_t iova, size_t len, > rte_mempool_populate_obj_cb_t *obj_cb, void *obj_cb_arg) > { > - size_t total_elt_sz; > - size_t off; > + struct rte_memzone *mz = obj_cb_arg; > + size_t total_elt_sz, off; Why two variables are combined into one here? It is unrelated change. > unsigned int i; > void *obj; > > total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size; > > for (off = 0, i = 0; off + total_elt_sz <= len && i < max_objs; i++) { > + > + /* Skip page boundary check if element is bigger than page */ > + if (mz->hugepage_sz >= total_elt_sz) { > + if (mempool_check_obj_bounds((char *)vaddr + off, > + mz->hugepage_sz, > + total_elt_sz) < 0) { > + i--; /* Decrement count & skip this obj */ > + off += total_elt_sz; > + continue; > + } > + } > + What I don't like here is that it makes one memory chunk insufficient to populate all objects. I.e. we calculated memory chunk size required, but skipped some object. May be it is not a problem, but breaks the logic existing in the code. > off += mp->header_size; > obj = (char *)vaddr + off; > obj_cb(mp, obj_cb_arg, obj,