[dpdk-dev,v4,6/7] mempool: introduce block size align flag

Message ID 20170815060743.21076-7-santosh.shukla@caviumnetworks.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Santosh Shukla Aug. 15, 2017, 6:07 a.m. UTC
  Some mempool hw like octeontx/fpa block, demands block size
(/total_elem_sz) aligned object start address.

Introducing an MEMPOOL_F_POOL_BLK_SZ_ALIGNED flag.
If this flag is set:
- Align object start address to a multiple of total_elt_sz.
- Allocate one additional object. Additional object is needed to make
  sure that requested 'n' object gets correctly populated. Example:

- Let's say that we get 'x' size of memory chunk from memzone.
- And application has requested 'n' object from mempool.
- Ideally, we start using objects at start address 0 to...(x-block_sz)
  for n obj.
- Not necessarily first object 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 start address of object is blk_sz
  aligned.
- Calculating 'off' may end up sacrificing first block_sz area of
  memzone area x. So total number of the object which can fit in the
  pool area is n-1, Which is incorrect behavior.

Therefore we request one additional object (/block_sz area) from memzone
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 | 16 +++++++++++++---
 lib/librte_mempool/rte_mempool.h |  1 +
 2 files changed, 14 insertions(+), 3 deletions(-)
  

Comments

Olivier Matz Sept. 4, 2017, 4:20 p.m. UTC | #1
On Tue, Aug 15, 2017 at 11:37:42AM +0530, Santosh Shukla wrote:
> Some mempool hw like octeontx/fpa block, demands block size
> (/total_elem_sz) aligned object start address.
> 
> Introducing an MEMPOOL_F_POOL_BLK_SZ_ALIGNED flag.
> If this flag is set:
> - Align object start address to a multiple of total_elt_sz.

Please specify if it's virtual or physical address.

What do you think about MEMPOOL_F_BLK_ALIGNED_OBJECTS instead?

I don't really like BLK because the word "block" is not used anywhere
else in the mempool code. But I cannot find any good replacement for
it. If you have another idea, please suggest.

> - Allocate one additional object. Additional object is needed to make
>   sure that requested 'n' object gets correctly populated. Example:
> 
> - Let's say that we get 'x' size of memory chunk from memzone.
> - And application has requested 'n' object from mempool.
> - Ideally, we start using objects at start address 0 to...(x-block_sz)
>   for n obj.
> - Not necessarily first object 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 start address of object is blk_sz
>   aligned.
> - Calculating 'off' may end up sacrificing first block_sz area of
>   memzone area x. So total number of the object which can fit in the
>   pool area is n-1, Which is incorrect behavior.
> 
> Therefore we request one additional object (/block_sz area) from memzone
> 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 | 16 +++++++++++++---
>  lib/librte_mempool/rte_mempool.h |  1 +
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 19e5e6ddf..7610f0d1f 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -239,10 +239,14 @@ 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,
> -		      __rte_unused const struct rte_mempool *mp)
> +		      const struct rte_mempool *mp)
>  {
>  	size_t obj_per_page, pg_num, pg_sz;
>  
> +	if (mp && mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED)
> +		/* alignment need one additional object */
> +		elt_num += 1;
> +
>  	if (total_elt_sz == 0)
>  		return 0;

I'm wondering if it's correct if the mempool area is not contiguous.

For instance:
 page size = 4096
 object size = 1900
 elt_num = 10

With your calculation, you will request (11+2-1)/2 = 6 pages.
But actually you may need 10 pages (max), since the number of object per
page matching the alignement constraint is 1, not 2.
  
Santosh Shukla Sept. 4, 2017, 5:45 p.m. UTC | #2
On Monday 04 September 2017 09:50 PM, Olivier MATZ wrote:
> On Tue, Aug 15, 2017 at 11:37:42AM +0530, Santosh Shukla wrote:
>> Some mempool hw like octeontx/fpa block, demands block size
>> (/total_elem_sz) aligned object start address.
>>
>> Introducing an MEMPOOL_F_POOL_BLK_SZ_ALIGNED flag.
>> If this flag is set:
>> - Align object start address to a multiple of total_elt_sz.
> Please specify if it's virtual or physical address.

virtual address. Yes will mention in v5. Thanks.

> What do you think about MEMPOOL_F_BLK_ALIGNED_OBJECTS instead?
>
> I don't really like BLK because the word "block" is not used anywhere
> else in the mempool code. But I cannot find any good replacement for
> it. If you have another idea, please suggest.
>
Ok with renaming to MEMPOOL_F_BLK_ALIGNED_OBJECTS

>> - Allocate one additional object. Additional object is needed to make
>>   sure that requested 'n' object gets correctly populated. Example:
>>
>> - Let's say that we get 'x' size of memory chunk from memzone.
>> - And application has requested 'n' object from mempool.
>> - Ideally, we start using objects at start address 0 to...(x-block_sz)
>>   for n obj.
>> - Not necessarily first object 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 start address of object is blk_sz
>>   aligned.
>> - Calculating 'off' may end up sacrificing first block_sz area of
>>   memzone area x. So total number of the object which can fit in the
>>   pool area is n-1, Which is incorrect behavior.
>>
>> Therefore we request one additional object (/block_sz area) from memzone
>> 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 | 16 +++++++++++++---
>>  lib/librte_mempool/rte_mempool.h |  1 +
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 19e5e6ddf..7610f0d1f 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -239,10 +239,14 @@ 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,
>> -		      __rte_unused const struct rte_mempool *mp)
>> +		      const struct rte_mempool *mp)
>>  {
>>  	size_t obj_per_page, pg_num, pg_sz;
>>  
>> +	if (mp && mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED)
>> +		/* alignment need one additional object */
>> +		elt_num += 1;
>> +
>>  	if (total_elt_sz == 0)
>>  		return 0;
> I'm wondering if it's correct if the mempool area is not contiguous.
>
> For instance:
>  page size = 4096
>  object size = 1900
>  elt_num = 10
>
> With your calculation, you will request (11+2-1)/2 = 6 pages.
> But actually you may need 10 pages (max), since the number of object per
> page matching the alignement constraint is 1, not 2.
>
In our case, we set PMD flag MEMPOOL_F_CAPA_PHYS_CONTIG to detect contiguity,
would fail at pool creation time, as HW don't support.
  
Olivier Matz Sept. 7, 2017, 7:27 a.m. UTC | #3
On Mon, Sep 04, 2017 at 11:15:50PM +0530, santosh wrote:
> 
> On Monday 04 September 2017 09:50 PM, Olivier MATZ wrote:
> > On Tue, Aug 15, 2017 at 11:37:42AM +0530, Santosh Shukla wrote:
> >> Some mempool hw like octeontx/fpa block, demands block size
> >> (/total_elem_sz) aligned object start address.
> >>
> >> Introducing an MEMPOOL_F_POOL_BLK_SZ_ALIGNED flag.
> >> If this flag is set:
> >> - Align object start address to a multiple of total_elt_sz.
> > Please specify if it's virtual or physical address.
> 
> virtual address. Yes will mention in v5. Thanks.
> 
> > What do you think about MEMPOOL_F_BLK_ALIGNED_OBJECTS instead?
> >
> > I don't really like BLK because the word "block" is not used anywhere
> > else in the mempool code. But I cannot find any good replacement for
> > it. If you have another idea, please suggest.
> >
> Ok with renaming to MEMPOOL_F_BLK_ALIGNED_OBJECTS
> 
> >> - Allocate one additional object. Additional object is needed to make
> >>   sure that requested 'n' object gets correctly populated. Example:
> >>
> >> - Let's say that we get 'x' size of memory chunk from memzone.
> >> - And application has requested 'n' object from mempool.
> >> - Ideally, we start using objects at start address 0 to...(x-block_sz)
> >>   for n obj.
> >> - Not necessarily first object 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 start address of object is blk_sz
> >>   aligned.
> >> - Calculating 'off' may end up sacrificing first block_sz area of
> >>   memzone area x. So total number of the object which can fit in the
> >>   pool area is n-1, Which is incorrect behavior.
> >>
> >> Therefore we request one additional object (/block_sz area) from memzone
> >> 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 | 16 +++++++++++++---
> >>  lib/librte_mempool/rte_mempool.h |  1 +
> >>  2 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> >> index 19e5e6ddf..7610f0d1f 100644
> >> --- a/lib/librte_mempool/rte_mempool.c
> >> +++ b/lib/librte_mempool/rte_mempool.c
> >> @@ -239,10 +239,14 @@ 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,
> >> -		      __rte_unused const struct rte_mempool *mp)
> >> +		      const struct rte_mempool *mp)
> >>  {
> >>  	size_t obj_per_page, pg_num, pg_sz;
> >>  
> >> +	if (mp && mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED)
> >> +		/* alignment need one additional object */
> >> +		elt_num += 1;
> >> +
> >>  	if (total_elt_sz == 0)
> >>  		return 0;
> > I'm wondering if it's correct if the mempool area is not contiguous.
> >
> > For instance:
> >  page size = 4096
> >  object size = 1900
> >  elt_num = 10
> >
> > With your calculation, you will request (11+2-1)/2 = 6 pages.
> > But actually you may need 10 pages (max), since the number of object per
> > page matching the alignement constraint is 1, not 2.
> >
> In our case, we set PMD flag MEMPOOL_F_CAPA_PHYS_CONTIG to detect contiguity,
> would fail at pool creation time, as HW don't support.

Yes but here it's generic code. If MEMPOOL_F_POOL_BLK_SZ_ALIGNED implies
MEMPOOL_F_CAPA_PHYS_CONTIG, it should be enforced somewhere.
  
Santosh Shukla Sept. 7, 2017, 7:37 a.m. UTC | #4
On Thursday 07 September 2017 12:57 PM, Olivier MATZ wrote:
> On Mon, Sep 04, 2017 at 11:15:50PM +0530, santosh wrote:
>> On Monday 04 September 2017 09:50 PM, Olivier MATZ wrote:
>>> On Tue, Aug 15, 2017 at 11:37:42AM +0530, Santosh Shukla wrote:
>>>> Some mempool hw like octeontx/fpa block, demands block size
>>>> (/total_elem_sz) aligned object start address.
>>>>
>>>> Introducing an MEMPOOL_F_POOL_BLK_SZ_ALIGNED flag.
>>>> If this flag is set:
>>>> - Align object start address to a multiple of total_elt_sz.
>>> Please specify if it's virtual or physical address.
>> virtual address. Yes will mention in v5. Thanks.
>>
>>> What do you think about MEMPOOL_F_BLK_ALIGNED_OBJECTS instead?
>>>
>>> I don't really like BLK because the word "block" is not used anywhere
>>> else in the mempool code. But I cannot find any good replacement for
>>> it. If you have another idea, please suggest.
>>>
>> Ok with renaming to MEMPOOL_F_BLK_ALIGNED_OBJECTS
>>
>>>> - Allocate one additional object. Additional object is needed to make
>>>>   sure that requested 'n' object gets correctly populated. Example:
>>>>
>>>> - Let's say that we get 'x' size of memory chunk from memzone.
>>>> - And application has requested 'n' object from mempool.
>>>> - Ideally, we start using objects at start address 0 to...(x-block_sz)
>>>>   for n obj.
>>>> - Not necessarily first object 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 start address of object is blk_sz
>>>>   aligned.
>>>> - Calculating 'off' may end up sacrificing first block_sz area of
>>>>   memzone area x. So total number of the object which can fit in the
>>>>   pool area is n-1, Which is incorrect behavior.
>>>>
>>>> Therefore we request one additional object (/block_sz area) from memzone
>>>> 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 | 16 +++++++++++++---
>>>>  lib/librte_mempool/rte_mempool.h |  1 +
>>>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>>>> index 19e5e6ddf..7610f0d1f 100644
>>>> --- a/lib/librte_mempool/rte_mempool.c
>>>> +++ b/lib/librte_mempool/rte_mempool.c
>>>> @@ -239,10 +239,14 @@ 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,
>>>> -		      __rte_unused const struct rte_mempool *mp)
>>>> +		      const struct rte_mempool *mp)
>>>>  {
>>>>  	size_t obj_per_page, pg_num, pg_sz;
>>>>  
>>>> +	if (mp && mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED)
>>>> +		/* alignment need one additional object */
>>>> +		elt_num += 1;
>>>> +
>>>>  	if (total_elt_sz == 0)
>>>>  		return 0;
>>> I'm wondering if it's correct if the mempool area is not contiguous.
>>>
>>> For instance:
>>>  page size = 4096
>>>  object size = 1900
>>>  elt_num = 10
>>>
>>> With your calculation, you will request (11+2-1)/2 = 6 pages.
>>> But actually you may need 10 pages (max), since the number of object per
>>> page matching the alignement constraint is 1, not 2.
>>>
>> In our case, we set PMD flag MEMPOOL_F_CAPA_PHYS_CONTIG to detect contiguity,
>> would fail at pool creation time, as HW don't support.
> Yes but here it's generic code. If MEMPOOL_F_POOL_BLK_SZ_ALIGNED implies
> MEMPOOL_F_CAPA_PHYS_CONTIG, it should be enforced somewhere.
>
Right, 
Approach:
We agree to keep _F_CAPA_PHYS_CONTIG as flag not set by application,
i'm thinking to keep that way for _ALIGNED flag too, both setted by mempool
handler, such that mempool handler sets both the flag before pool creation time.
And above condition will check for both the flags? are you fine with that approach?
Or pl. suggest an alternative? As because right now octeontx mempool block _need_
aligned blocks and cares for contiguity. Having said that ext-mempool in particular
and mempool in general doesn't facilitate anything like that..

Thanks.
  

Patch

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 19e5e6ddf..7610f0d1f 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -239,10 +239,14 @@  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,
-		      __rte_unused const struct rte_mempool *mp)
+		      const struct rte_mempool *mp)
 {
 	size_t obj_per_page, pg_num, pg_sz;
 
+	if (mp && mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED)
+		/* alignment need one additional object */
+		elt_num += 1;
+
 	if (total_elt_sz == 0)
 		return 0;
 
@@ -265,13 +269,16 @@  rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift,
 ssize_t
 rte_mempool_xmem_usage(__rte_unused void *vaddr, uint32_t elt_num,
 	size_t total_elt_sz, const phys_addr_t paddr[], uint32_t pg_num,
-	uint32_t pg_shift, __rte_unused const struct rte_mempool *mp)
+	uint32_t pg_shift, const struct rte_mempool *mp)
 {
 	uint32_t elt_cnt = 0;
 	phys_addr_t start, end;
 	uint32_t paddr_idx;
 	size_t pg_sz = (size_t)1 << pg_shift;
 
+	if (mp && mp->flags & MEMPOOL_F_POOL_BLK_SZ_ALIGNED)
+		/* alignment need one additional object */
+		elt_num += 1;
 
 	/* if paddr is NULL, assume contiguous memory */
 	if (paddr == NULL) {
@@ -389,7 +396,10 @@  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)
+		/* align object start address to a multiple of total_elt_sz */
+		off = total_elt_sz - ((uintptr_t)vaddr % total_elt_sz);
+	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;
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index a4bfdb56e..d7c2416f4 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -266,6 +266,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_CAPA_PHYS_CONTIG 0x0040 /**< Detect physcially contiguous objs */
+#define MEMPOOL_F_POOL_BLK_SZ_ALIGNED 0x0080 /**< Align obj start address to total elem size */
 
 /**
  * @internal When debug is enabled, store some statistics.