DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier Matz <olivier.matz@6wind.com>
To: santosh <santosh.shukla@caviumnetworks.com>
Cc: dev@dpdk.org, thomas@monjalon.net, hemant.agrawal@nxp.com,
	jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [PATCH 3/4] mempool: introduce block size align flag
Date: Mon, 10 Jul 2017 15:15:11 +0200	[thread overview]
Message-ID: <20170710151511.7972e83d@platinum> (raw)
In-Reply-To: <f3e46698-90f1-5dd2-260a-fadc14c3d2fd@caviumnetworks.com>

On Wed, 5 Jul 2017 13:05:57 +0530, santosh <santosh.shukla@caviumnetworks.com> wrote:
> Hi Olivier,
> 
> On Monday 03 July 2017 10:07 PM, Olivier Matz wrote:
> 
> > On Wed, 21 Jun 2017 17:32:47 +0000, Santosh Shukla <santosh.shukla@caviumnetworks.com> wrote:  
> >> Some mempool hw like octeontx/fpa block, demands block size aligned
> >> buffer address.
> >>  
> > What is the meaning of block size aligned?  
> 
> block size is total_elem_sz.
> 
> > Does it mean that the address has to be a multiple of total_elt_size?  
> 
> yes.
> 
> > Is this constraint on the virtual address only?
> >  
> both.

You mean virtual address and physical address must be a multiple of
total_elt_size? How is it possible?

For instance, I have this on my dpdk instance:
Segment 0: phys:0x52c00000, len:4194304, virt:0x7fed26000000, socket_id:0, hugepage_sz:2097152, nchannel:0, nrank:0
Segment 1: phys:0x53400000, len:163577856, virt:0x7fed1c200000, socket_id:0, hugepage_sz:2097152, nchannel:0, nrank:0
Segment 2: phys:0x5d400000, len:20971520, virt:0x7fed1ac00000, socket_id:0, hugepage_sz:2097152, nchannel:0, nrank:0
...

I assume total_elt_size is 2176.

In segment 0, I need to use (virt = 0x7fed26000000 + 1536) to be
multiple of 2176. But the corresponding physical address (0x52c00000 + 1536)
is not multiple of 2176. 


Please clarify if only the virtual address has to be aligned.


> 
> >> 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.
> >  
> which part?
> 
> I'll try to reword.
> 
> The problem statement is:
> - We want start of buffer address aligned to block_sz aka total_elt_sz.
> 
> Proposed solution in this patch:
> - Let's say that we get 'x' size of memory chunk from memzone.
> - Ideally we start using buffer at address 0 to...(x-block_sz).
> - Not necessarily first buffer address i.e. 0 is aligned to block_sz.
> - So we derive offset value for block_sz alignment purpose i.e..'off' . 
> - That 'off' makes sure that first va/pa address of buffer is blk_sz aligned.
> - Calculating 'off' may end up sacrificing first buffer of pool. So total
> number of buffer in pool is n-1, Which is incorrect behavior, Thats why
> we add 1 addition buffer. We request memzone to allocate (n+1 * total_elt_sz) pool
> area when F_BLK_SZ_ALIGNED flag is set.
> 
> >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> >> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >> ---
> >>  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?  
> 
> Purpose is to sanity check blk_sz alignment. No it won;t fail.
> I thought better to keep sanity check but if you see no value
> then will remove in v2?

yes please


> > 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.  
> 
> ok, in v2. 
> 
> >> @@ -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.
> >  
> Per my description on [1/4]. If we agree to call
> _ops_get_capability() at very beginning i.e.. at _populate_default()
> then 'mp->flag' has capability flag. and We could add one more argument
> in _xmem_size( , flag)/_xmem_usage(, flag). 
> - xmem_size / xmem_usage() to check for that capability bit in 'flag'. 
> - if set then increase 'elt_num' by num.
> 
> That way your approach 2) make sense to me and it will very well fit
> in design. Won't waste memory like you mentioned in approach 1).
> 
> Does that make sense?

The use case of rte_mempool_xmem_size()/rte_mempool_xmem_usage()
is to determine how much memory is needed to instanciate a mempool:

  sz = rte_mempool_xmem_size(...);
  ptr = allocate(sz);
  paddr_table = get_phys_map(ptr);
  mp = rte_mempool_xmem_create(..., ptr, ..., paddr_table, ...);

If we want to transform the code to use another mempool_ops, it is
not possible:

  mp = rte_mempool_create_empty();
  rte_mempool_set_ops_byname(mp, "my-handler");
  sz = rte_mempool_xmem_size(...);   /* <<< the mp pointer is not passed */
  ptr = allocate(sz);
  paddr_table = get_phys_map(ptr);
  mp = rte_mempool_xmem_create(..., ptr, ..., paddr_table, ...);

So, yes, this approach would work but it needs to change the API.
I think it is possible to keep a compat with previous versions.

 
> >> 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.
> >  
> I won't see this as HW limitation. As mentioned in [1/4], even application
> can request for block alignment, right?

What would be the reason for an application would request this block aligment?
The total size of the element is usually not known by the application,
because the mempool adds its header and footer.

  reply	other threads:[~2017-07-10 13:15 UTC|newest]

Thread overview: 116+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-21 17:32 [dpdk-dev] [PATCH 0/4] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-06-21 17:32 ` [dpdk-dev] [PATCH 1/4] mempool: get the external mempool capability Santosh Shukla
2017-07-03 16:37   ` Olivier Matz
2017-07-05  6:41     ` santosh
2017-07-10 13:55       ` Olivier Matz
2017-07-10 16:09         ` santosh
2017-06-21 17:32 ` [dpdk-dev] [PATCH 2/4] mempool: detect physical contiguous object in pool Santosh Shukla
2017-07-03 16:37   ` Olivier Matz
2017-07-05  7:07     ` santosh
2017-06-21 17:32 ` [dpdk-dev] [PATCH 3/4] mempool: introduce block size align flag Santosh Shukla
2017-07-03 16:37   ` Olivier Matz
2017-07-05  7:35     ` santosh
2017-07-10 13:15       ` Olivier Matz [this message]
2017-07-10 16:22         ` santosh
2017-06-21 17:32 ` [dpdk-dev] [PATCH 4/4] mempool: update range info to pool Santosh Shukla
2017-07-13  9:32 ` [dpdk-dev] [PATCH v2 0/6] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 1/6] mempool: fix flags data type Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 2/6] mempool: get the mempool capability Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 3/6] mempool: detect physical contiguous object in pool Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 4/6] mempool: add mempool arg in xmem size and usage Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 5/6] mempool: introduce block size align flag Santosh Shukla
2017-07-13  9:32   ` [dpdk-dev] [PATCH v2 6/6] mempool: update range info to pool Santosh Shukla
2017-07-18  6:07   ` [dpdk-dev] [PATCH v2 0/6] Infrastructure to support octeontx HW mempool manager santosh
2017-07-20 13:47   ` [dpdk-dev] [PATCH v3 " Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 1/6] mempool: fix flags data type Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 2/6] mempool: get the mempool capability Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 3/6] mempool: detect physical contiguous object in pool Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 4/6] mempool: add mempool arg in xmem size and usage Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 5/6] mempool: introduce block size align flag Santosh Shukla
2017-07-20 13:47     ` [dpdk-dev] [PATCH v3 6/6] mempool: update range info to pool Santosh Shukla
2017-08-15  6:07     ` [dpdk-dev] [PATCH v4 0/7] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 1/7] mempool: fix flags data type Santosh Shukla
2017-09-04 14:11         ` Olivier MATZ
2017-09-04 14:18           ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 2/7] mempool: add mempool arg in xmem size and usage Santosh Shukla
2017-09-04 14:22         ` Olivier MATZ
2017-09-04 14:33           ` santosh
2017-09-04 14:46             ` Olivier MATZ
2017-09-04 14:58               ` santosh
2017-09-04 15:23                 ` Olivier MATZ
2017-09-04 15:52                   ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 3/7] doc: remove mempool api change notice Santosh Shukla
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 4/7] mempool: get the mempool capability Santosh Shukla
2017-09-04 14:32         ` Olivier MATZ
2017-09-04 14:44           ` santosh
2017-09-04 15:56             ` Olivier MATZ
2017-09-04 16:29               ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 5/7] mempool: detect physical contiguous object in pool Santosh Shukla
2017-09-04 14:43         ` Olivier MATZ
2017-09-04 14:47           ` santosh
2017-09-04 16:00             ` Olivier MATZ
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 6/7] mempool: introduce block size align flag Santosh Shukla
2017-09-04 16:20         ` Olivier MATZ
2017-09-04 17:45           ` santosh
2017-09-07  7:27             ` Olivier MATZ
2017-09-07  7:37               ` santosh
2017-08-15  6:07       ` [dpdk-dev] [PATCH v4 7/7] mempool: update range info to pool Santosh Shukla
2017-09-06 11:28       ` [dpdk-dev] [PATCH v5 0/8] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 1/8] mempool: remove unused flags argument Santosh Shukla
2017-09-07  7:41           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 2/8] mempool: change flags from int to unsigned int Santosh Shukla
2017-09-07  7:43           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 3/8] mempool: add flags arg in xmem size and usage Santosh Shukla
2017-09-07  7:46           ` Olivier MATZ
2017-09-07  7:49             ` santosh
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 4/8] doc: remove mempool notice Santosh Shukla
2017-09-07  7:47           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 5/8] mempool: get the mempool capability Santosh Shukla
2017-09-07  7:59           ` Olivier MATZ
2017-09-07  8:15             ` santosh
2017-09-07  8:39               ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 6/8] mempool: detect physical contiguous object in pool Santosh Shukla
2017-09-07  8:05           ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 7/8] mempool: introduce block size align flag Santosh Shukla
2017-09-07  8:13           ` Olivier MATZ
2017-09-07  8:27             ` santosh
2017-09-07  8:57               ` Olivier MATZ
2017-09-06 11:28         ` [dpdk-dev] [PATCH v5 8/8] mempool: update range info to pool Santosh Shukla
2017-09-07  8:30           ` Olivier MATZ
2017-09-07  8:56             ` santosh
2017-09-07  9:09               ` Olivier MATZ
2017-09-07 15:30         ` [dpdk-dev] [PATCH v6 0/8] Infrastructure to support octeontx HW mempool manager Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 1/8] mempool: remove unused flags argument Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 2/8] mempool: change flags from int to unsigned int Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 3/8] mempool: add flags arg in xmem size and usage Santosh Shukla
2017-09-25 11:24             ` Olivier MATZ
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 4/8] doc: remove mempool notice Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 5/8] mempool: get the mempool capability Santosh Shukla
2017-09-25 11:26             ` Olivier MATZ
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 6/8] mempool: detect physical contiguous object in pool Santosh Shukla
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 7/8] mempool: introduce block size align flag Santosh Shukla
2017-09-22 12:59             ` Hemant Agrawal
2017-09-25 11:32             ` Olivier MATZ
2017-09-25 22:08               ` santosh
2017-09-07 15:30           ` [dpdk-dev] [PATCH v6 8/8] mempool: notify memory area to pool Santosh Shukla
2017-09-25 11:41             ` Olivier MATZ
2017-09-25 22:18               ` santosh
2017-09-29  4:53                 ` santosh
2017-09-29  8:20                   ` Olivier MATZ
2017-09-29  8:25                     ` santosh
2017-09-13  9:58           ` [dpdk-dev] [PATCH v6 0/8] Infrastructure to support octeontx HW mempool manager santosh
2017-09-19  8:26             ` santosh
2017-10-01  9:28           ` [dpdk-dev] [PATCH v7 " Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 1/8] mempool: remove unused flags argument Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 2/8] mempool: change flags from int to unsigned int Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 3/8] mempool: add flags arg in xmem size and usage Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 4/8] doc: remove mempool notice Santosh Shukla
2017-10-01  9:28             ` [dpdk-dev] [PATCH v7 5/8] mempool: get the mempool capability Santosh Shukla
2017-10-01  9:29             ` [dpdk-dev] [PATCH v7 6/8] mempool: detect physical contiguous object in pool Santosh Shukla
2017-10-01  9:29             ` [dpdk-dev] [PATCH v7 7/8] mempool: introduce block size align flag Santosh Shukla
2017-10-02  8:35               ` santosh
2017-10-02 14:26               ` Olivier MATZ
2017-10-01  9:29             ` [dpdk-dev] [PATCH v7 8/8] mempool: notify memory area to pool Santosh Shukla
2017-10-02  8:36               ` santosh
2017-10-02 14:27               ` Olivier MATZ
2017-10-06 20:00             ` [dpdk-dev] [PATCH v7 0/8] Infrastructure to support octeontx HW mempool manager Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170710151511.7972e83d@platinum \
    --to=olivier.matz@6wind.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerin.jacob@caviumnetworks.com \
    --cc=santosh.shukla@caviumnetworks.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).