From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id C3FA71B880 for ; Thu, 1 Feb 2018 09:51:38 +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-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id D9434940054; Thu, 1 Feb 2018 08:51:36 +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 08:51:32 +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-5-git-send-email-arybchenko@solarflare.com> <20180131164529.laku7jdh3hgriall@platinum> From: Andrew Rybchenko Message-ID: <4277a6f7-0f4e-b778-f829-849051814dd8@solarflare.com> Date: Thu, 1 Feb 2018 11:51:27 +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: <20180131164529.laku7jdh3hgriall@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--17.125100-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1517475097-gKGqsRl2sx+l 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 04/17] mempool: add op to populate objects using provided memory 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 08:51:39 -0000 On 01/31/2018 07:45 PM, Olivier Matz wrote: > On Tue, Jan 23, 2018 at 01:15:59PM +0000, Andrew Rybchenko wrote: >> The callback allows to customize how objects are stored in the >> memory chunk. Default implementation of the callback which simply >> puts objects one by one is available. >> >> Suggested-by: Olivier Matz >> Signed-off-by: Andrew Rybchenko > ... > >> >> +int >> +rte_mempool_populate_one_by_one(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) > We shall find a better name for this function. > Unfortunatly rte_mempool_populate_default() already exists... I have no better idea right now, but we'll try in the next version. May be rte_mempool_op_populate_default()? > I'm also wondering if having a file rte_mempool_ops_default.c > with all the default behaviors would make sense? I think it is a good idea. Will do. > ... > >> @@ -466,16 +487,13 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr, >> else >> off = RTE_PTR_ALIGN_CEIL(vaddr, RTE_CACHE_LINE_SIZE) - vaddr; >> >> - while (off + total_elt_sz <= len && mp->populated_size < mp->size) { >> - off += mp->header_size; >> - if (iova == RTE_BAD_IOVA) >> - mempool_add_elem(mp, (char *)vaddr + off, >> - RTE_BAD_IOVA); >> - else >> - mempool_add_elem(mp, (char *)vaddr + off, iova + off); >> - off += mp->elt_size + mp->trailer_size; >> - i++; >> - } >> + if (off > len) >> + return -EINVAL; >> + >> + 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); > My initial idea was to provide populate_iova(), populate_virt(), ... > as mempool ops. I don't see any strong requirement for doing it now, but > on the other hand it would break the API to do it later. What's > your opinion? Suggested solution keeps only generic house-keeping inside rte_mempool_populate_iova() (driver-data alloc/init, generic check if the pool is already populated, maintenance of the memory chunks list and object cache-alignment requirements). I think that only the last item is questionable, but cache-line alignment is hard-wired in object size calculation as well which is not customizable yet. May be we should add callback for object size calculation with default fallback and move object cache-line alignment into populate() callback. As for populate_virt() etc right now all these functions finally come to populate_iova(). I have no customization usecases for these functions in my mind, so it is hard to guess required set of parameters. That's why I kept it as is for now. (In general I prefer to avoid overkill solutions since chances of success (100% guess of the prototype) are small) May be someone else on the list have usecases in mind? > Also, I see that mempool_add_elem() is passed as a callback to > rte_mempool_ops_populate(). Instead, would it make sense to > export mempool_add_elem() and let the implementation of populate() > ops to call it? I think callback gives a bit more freedom and allows to pass own function which does some actions (e.g. filtering) per object. In fact I think opaque parameter should be added to the callback prototype to make it really useful for customization (to provide specific context and make it possible to chain callbacks).