From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f179.google.com (mail-wr0-f179.google.com [209.85.128.179]) by dpdk.org (Postfix) with ESMTP id 8F6A75689 for ; Mon, 3 Jul 2017 18:37:23 +0200 (CEST) Received: by mail-wr0-f179.google.com with SMTP id 77so237184548wrb.1 for ; Mon, 03 Jul 2017 09:37:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=5RH9WTQr5CsQsck3wDUZAHOTTCW3T0tG6ABXSSI/4io=; b=VomqE/DrHD3JSFBw+LJ/VtQYkZwdMKEwqCuDAHJue0Ca3/BKUhXrFiCW8ELSBMQsJA 5lvtTvH5NUV8E/5Y+9uweNprvCxEV2DkTcWfkvlgQfRwjCuokm2Jvmhq2MWarRsCy4KO Gc6Pau/WcuRecbXskNebiarlmKmJ7QezCxsYIVisyFIol4RVl8sYcbZ4biSiVknlI1ro 9iXHSssz8LOSCH8nCDNU5a/ftvAoWj1oO/DMmDqEDTVHs8PDSziUnqkfXK7Wwj6FAAJe IGOH25Z3iE6/Jpmh8Da3YJFZd5fo++DNoTn284y5UNAPodmP8sk7DdYgWsxi8jnwAjWa S84w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=5RH9WTQr5CsQsck3wDUZAHOTTCW3T0tG6ABXSSI/4io=; b=gkooSV14RY/gbjPMJpWs6Kt5vVEGZ2NlQIyOzH3cjXW2fBeG800cnY7foWksYrLEwY obQLwkGOoHBFTXtQ2e2BovuE0bMNEUpgyfX98NnJ8gKLbP3MCIQhUrz+PxB18xZEC1kK Ufptpr0B3UuO96niORNnojqqkHtMThYo9Yznt/EbH0iPZLFLZuLkTDQEZ6RUPJeo4QTf lrFUTEg8jF2ylRBTMNUd6WFLLsu7EQlqGAwkorD0G/sP6HJiI6L5n3wyeZzY6Dvt8PHj OhatXCy4rGrJ7/MqV0l8r8tUs2RUPBwQ3hYqwHIe9Jw5Mqw2gZxJ4Z/BQ9/JaNQXELE6 osdw== X-Gm-Message-State: AKS2vOz4mG/5X1Rcsyf52ZYue/WCEA6CnYs/Ad/4+GRv5GgEkIDUweXg ORUu6+3GOXnZDNMK X-Received: by 10.223.172.15 with SMTP id v15mr38924494wrc.84.1499099843103; Mon, 03 Jul 2017 09:37:23 -0700 (PDT) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id f128sm6229510wmd.34.2017.07.03.09.37.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 03 Jul 2017 09:37:23 -0700 (PDT) Date: Mon, 3 Jul 2017 18:37:20 +0200 From: Olivier Matz To: Santosh Shukla Cc: dev@dpdk.org, thomas@monjalon.net, hemant.agrawal@nxp.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com Message-ID: <20170703183720.381369de@platinum> In-Reply-To: <20170621173248.1313-4-santosh.shukla@caviumnetworks.com> References: <20170621173248.1313-1-santosh.shukla@caviumnetworks.com> <20170621173248.1313-4-santosh.shukla@caviumnetworks.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 3/4] mempool: introduce block size align flag 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: Mon, 03 Jul 2017 16:37:23 -0000 On Wed, 21 Jun 2017 17:32:47 +0000, Santosh Shukla wrote: > Some mempool hw like octeontx/fpa block, demands block size aligned > buffer address. > What is the meaning of block size aligned? Does it mean that the address has to be a multiple of total_elt_size? Is this constraint on the virtual address only? > Introducing an MEMPOOL_F_POOL_BLK_SZ_ALIGNED flag. > If this flag is set: > 1) adjust 'off' value to block size aligned value. > 2) Allocate one additional buffer. This buffer is used to make sure that > requested 'n' buffers get correctly populated to mempool. > Example: > elem_sz = 2432 // total element size. > n = 2111 // requested number of buffer. > off = 2304 // new buf_offset value after step 1) > vaddr = 0x0 // actual start address of pool > pool_len = 5133952 // total pool length i.e.. (elem_sz * n) > > Since 'off' is a non-zero value so below condition would fail for the > block size align case. > > (((vaddr + off) + (elem_sz * n)) <= (vaddr + pool_len)) > > Which is incorrect behavior. Additional buffer will solve this > problem and correctly populate 'n' buffer to mempool for the aligned > mode. Sorry, but the example is not very clear. > > Signed-off-by: Santosh Shukla > Signed-off-by: Jerin Jacob > --- > lib/librte_mempool/rte_mempool.c | 19 ++++++++++++++++--- > lib/librte_mempool/rte_mempool.h | 1 + > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index 7dec2f51d..2010857f0 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -350,7 +350,7 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr, > { > unsigned total_elt_sz; > unsigned i = 0; > - size_t off; > + size_t off, delta; > struct rte_mempool_memhdr *memhdr; > int ret; > > @@ -387,7 +387,15 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr, > memhdr->free_cb = free_cb; > memhdr->opaque = opaque; > > - if (mp->flags & MEMPOOL_F_NO_CACHE_ALIGN) > + if (mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED) { > + delta = (uintptr_t)vaddr % total_elt_sz; > + off = total_elt_sz - delta; > + /* Validate alignment */ > + if (((uintptr_t)vaddr + off) % total_elt_sz) { > + RTE_LOG(ERR, MEMPOOL, "vaddr(%p) not aligned to total_elt_sz(%u)\n", (vaddr + off), total_elt_sz); > + return -EINVAL; > + } > + } else if (mp->flags & MEMPOOL_F_NO_CACHE_ALIGN) > off = RTE_PTR_ALIGN_CEIL(vaddr, 8) - vaddr; > else > off = RTE_PTR_ALIGN_CEIL(vaddr, RTE_CACHE_LINE_SIZE) - vaddr; What is the purpose of this test? Can it fail? Not sure having the delta variable is helpful. However, adding a small comment like this could help: /* align object start address to a multiple of total_elt_sz */ off = total_elt_sz - ((uintptr_t)vaddr % total_elt_sz); About style, please don't mix brackets and no-bracket blocks in the same if/elseif/else. > @@ -555,8 +563,13 @@ rte_mempool_populate_default(struct rte_mempool *mp) > } > > total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size; > + > for (mz_id = 0, n = mp->size; n > 0; mz_id++, n -= ret) { > - size = rte_mempool_xmem_size(n, total_elt_sz, pg_shift); > + if (mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED) > + size = rte_mempool_xmem_size(n + 1, total_elt_sz, > + pg_shift); > + else > + size = rte_mempool_xmem_size(n, total_elt_sz, pg_shift); > > ret = snprintf(mz_name, sizeof(mz_name), > RTE_MEMPOOL_MZ_FORMAT "_%d", mp->name, mz_id); One issue I see here is that this new flag breaks the function rte_mempool_xmem_size(), which calculates the maximum amount of memory required to store a given number of objects. It also probably breaks rte_mempool_xmem_usage(). I don't have any good solution for now. A possibility is to change the behavior of these functions for everyone, meaning that we will always reserve more memory that really required. If this is done on every memory chunk (struct rte_mempool_memhdr), it can eat a lot of memory. Another approach would be to change the API of this function to pass the capability flags, or the mempool pointer... but there is a problem because these functions are usually called before the mempool is instanciated. > diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h > index fd8722e69..99a20263d 100644 > --- a/lib/librte_mempool/rte_mempool.h > +++ b/lib/librte_mempool/rte_mempool.h > @@ -267,6 +267,7 @@ struct rte_mempool { > #define MEMPOOL_F_POOL_CREATED 0x0010 /**< Internal: pool is created. */ > #define MEMPOOL_F_NO_PHYS_CONTIG 0x0020 /**< Don't need physically contiguous objs. */ > #define MEMPOOL_F_POOL_CONTIG 0x0040 /**< Detect physcially contiguous objs */ > +#define MEMPOOL_F_POOL_BLK_SZ_ALIGNED 0x0080 /**< Align buffer address to block size*/ > > /** > * @internal When debug is enabled, store some statistics. Same comment than for patch 3: the explanation should really be clarified. It's a hw specific limitation, which won't be obvious for the people that will read that code, so we must document it as clear as possible.