From mboxrd@z Thu Jan 1 00:00:00 1970 From: santosh Subject: Re: [RFC PATCH 2/6] mempool: implement clustered object allocation Date: Wed, 17 Jan 2018 21:25:43 +0530 Message-ID: <69c425a0-7339-3e7a-a0d2-e2987a644cf2@caviumnetworks.com> References: <1511539591-20966-1-git-send-email-arybchenko@solarflare.com> <1511539591-20966-3-git-send-email-arybchenko@solarflare.com> <20171214133721.b7z2i7yccwpath37@platinum> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, "Artem V. Andreev" To: Andrew Rybchenko , Olivier MATZ Return-path: Received: from NAM01-BN3-obe.outbound.protection.outlook.com (mail-bn3nam01on0068.outbound.protection.outlook.com [104.47.33.68]) by dpdk.org (Postfix) with ESMTP id CE55CA48B for ; Wed, 17 Jan 2018 16:55:58 +0100 (CET) In-Reply-To: Content-Language: en-US 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 Wednesday 17 January 2018 08:33 PM, Andrew Rybchenko wrote: > On 12/14/2017 04:37 PM, Olivier MATZ wrote: >> On Fri, Nov 24, 2017 at 04:06:27PM +0000, Andrew Rybchenko wrote: >>> From: "Artem V. Andreev" >>> >>> Clustered allocation is required to simplify packaging objects into >>> buckets and search of the bucket control structure by an object. >>> >>> Signed-off-by: Artem V. Andreev >>> Signed-off-by: Andrew Rybchenko >>> --- >>>   lib/librte_mempool/rte_mempool.c | 39 +++++++++++++++++++++++++++++++++++---- >>>   lib/librte_mempool/rte_mempool.h | 23 +++++++++++++++++++++-- >>>   test/test/test_mempool.c         |  2 +- >>>   3 files changed, 57 insertions(+), 7 deletions(-) >>> >>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c >>> index d50dba4..43455a3 100644 >>> --- a/lib/librte_mempool/rte_mempool.c >>> +++ b/lib/librte_mempool/rte_mempool.c >>> @@ -239,7 +239,8 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags, >>>    */ >>>   size_t >>>   rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift, >>> -              unsigned int flags) >>> +              unsigned int flags, >>> +              const struct rte_mempool_info *info) >>>   { >>>       size_t obj_per_page, pg_num, pg_sz; >>>       unsigned int mask; >>> @@ -252,6 +253,17 @@ rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift, >>>       if (total_elt_sz == 0) >>>           return 0; >>>   +    if (flags & MEMPOOL_F_CAPA_ALLOCATE_IN_CLUSTERS) { >>> +        unsigned int align_shift = >>> +            rte_bsf32( >>> +                rte_align32pow2(total_elt_sz * >>> +                        info->cluster_size)); >>> +        if (pg_shift < align_shift) { >>> +            return ((elt_num / info->cluster_size) + 2) >>> +                << align_shift; >>> +        } >>> +    } >>> + >> +Cc Santosh for this >> >> To be honnest, that was my fear when introducing >> MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS and MEMPOOL_F_CAPA_PHYS_CONTIG to see more >> and more specific flags in generic code. >> >> I feel that the hidden meaning of these flags is more "if driver == foo", >> which shows that something is wrong is the current design. >> >> We have to think about another way to do. Let me try to propose >> something (to be deepen). >> >> The standard way to create a mempool is: >> >>    mp = create_empty(...) >>    set_ops_by_name(mp, "my-driver")    // optional >>    populate_default(mp)                // or populate_*() >>    obj_iter(mp, callback, arg)         // optional, to init objects >>    // and optional local func to init mempool priv >> >> First, we can consider deprecating some APIs like: >>   - rte_mempool_xmem_create() >>   - rte_mempool_xmem_size() >>   - rte_mempool_xmem_usage() >>   - rte_mempool_populate_iova_tab() >> >> These functions were introduced for xen, which was recently >> removed. They are complex to use, and are not used anywhere else in >> DPDK. >> >> Then, instead of having flags (quite hard to understand without knowing >> the underlying driver), we can let the mempool drivers do the >> populate_default() operation. For that we can add a populate_default >> field in mempool ops. Same for populate_virt(), populate_anon(), and >> populate_phys() which can return -ENOTSUP if this is not >> implemented/implementable on a specific driver, or if flags >> (NO_CACHE_ALIGN, NO_SPREAD, ...) are not supported. If the function >> pointer is NULL, use the generic function. >> >> Thanks to this, the generic code would remain understandable and won't >> have to care about how memory should be allocated for a specific driver. >> >> Thoughts? > > Yes, I agree. This week we'll provide updated version of the RFC which > covers it including transition of the mempool/octeontx. I think it is sufficient > to introduce two new ops: >  1. To calculate memory space required to store specified number of objects >  2. To populate objects in the provided memory chunk (the op will be called >      from rte_mempool_populate_iova() which is a leaf function for all >      rte_mempool_populate_*() calls. > It will allow to avoid duplication and keep memchunks housekeeping inside > mempool library. > There is also a downside of letting mempool driver to populate, which was raised in other thread. http://dpdk.org/dev/patchwork/patch/31943/ Thanks.