[dpdk-dev,v5,7/8] mempool: introduce block size align flag
diff mbox

Message ID 20170906112834.32378-8-santosh.shukla@caviumnetworks.com
State Superseded, archived
Headers show

Checks

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

Commit Message

Santosh Shukla Sept. 6, 2017, 11:28 a.m. 
Some mempool hw like octeontx/fpa block, demands block size
(/total_elem_sz) aligned object start address.

Introducing an MEMPOOL_F_BLK_ALIGNED_OBJECTS flag.
If this flag is set:
- Align object start address(vaddr) 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_ALIGNED_OBJECTS flag is set.

Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
v4 --> v5:
- Added vaddr in git description of patch (suggested by Olivier)
- Renamed to aligned flag to MEMPOOL_F_BLK_ALIGNED_OBJECTS (suggested by
  Olivier)
Refer [1].
[1] http://dpdk.org/dev/patchwork/patch/27600/

 lib/librte_mempool/rte_mempool.c | 17 ++++++++++++++---
 lib/librte_mempool/rte_mempool.h |  4 ++++
 2 files changed, 18 insertions(+), 3 deletions(-)

Comments

Olivier Matz Sept. 7, 2017, 8:13 a.m. | #1 
On Wed, Sep 06, 2017 at 04:58:33PM +0530, Santosh Shukla wrote:
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -271,6 +271,10 @@ struct rte_mempool {
>   * Note: This flag should not be passed by application.
>   */
>  #define MEMPOOL_F_CAPA_PHYS_CONTIG 0x0040
> +/**
> + * Align object start address to total elem size
> + */
> +#define MEMPOOL_F_BLK_ALIGNED_OBJECTS 0x0080

Same than with the other flag: since the meaning of this flag is not obvious
when we read the name, it has to be clearly described.
- say that it's virtual address
- say that it implies MEMPOOL_F_CAPA_PHYS_CONTIG
- say that it can be advertised by a driver and the application should
  not pass it

And, since it shall not be passed by an application, I suggest to add
_CAPA too (i.e. MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS).
Santosh Shukla Sept. 7, 2017, 8:27 a.m. | #2 
On Thursday 07 September 2017 01:43 PM, Olivier MATZ wrote:
> On Wed, Sep 06, 2017 at 04:58:33PM +0530, Santosh Shukla wrote:
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -271,6 +271,10 @@ struct rte_mempool {
>>   * Note: This flag should not be passed by application.
>>   */
>>  #define MEMPOOL_F_CAPA_PHYS_CONTIG 0x0040
>> +/**
>> + * Align object start address to total elem size
>> + */
>> +#define MEMPOOL_F_BLK_ALIGNED_OBJECTS 0x0080
> Same than with the other flag: since the meaning of this flag is not obvious
> when we read the name, it has to be clearly described.
> - say that it's virtual address
> - say that it implies MEMPOOL_F_CAPA_PHYS_CONTIG
> - say that it can be advertised by a driver and the application should
>   not pass it
>
> And, since it shall not be passed by an application, I suggest to add
> _CAPA too (i.e. MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS).
>
Ok, I will elaborate on FLAG description in v6,
and Rename to MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS.

Can you please suggest are you ok with
checking MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS | _PHYS_CONTIG
in _xmem_size()/_usage(), asked in v4 [1] for same patch.

[1] http://dpdk.org/dev/patchwork/patch/27600/
Olivier Matz Sept. 7, 2017, 8:57 a.m. | #3 
On Thu, Sep 07, 2017 at 01:57:57PM +0530, santosh wrote:
> 
> On Thursday 07 September 2017 01:43 PM, Olivier MATZ wrote:
> > On Wed, Sep 06, 2017 at 04:58:33PM +0530, Santosh Shukla wrote:
> >> --- a/lib/librte_mempool/rte_mempool.h
> >> +++ b/lib/librte_mempool/rte_mempool.h
> >> @@ -271,6 +271,10 @@ struct rte_mempool {
> >>   * Note: This flag should not be passed by application.
> >>   */
> >>  #define MEMPOOL_F_CAPA_PHYS_CONTIG 0x0040
> >> +/**
> >> + * Align object start address to total elem size
> >> + */
> >> +#define MEMPOOL_F_BLK_ALIGNED_OBJECTS 0x0080
> > Same than with the other flag: since the meaning of this flag is not obvious
> > when we read the name, it has to be clearly described.
> > - say that it's virtual address
> > - say that it implies MEMPOOL_F_CAPA_PHYS_CONTIG
> > - say that it can be advertised by a driver and the application should
> >   not pass it
> >
> > And, since it shall not be passed by an application, I suggest to add
> > _CAPA too (i.e. MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS).
> >
> Ok, I will elaborate on FLAG description in v6,
> and Rename to MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS.
> 
> Can you please suggest are you ok with
> checking MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS | _PHYS_CONTIG
> in _xmem_size()/_usage(), asked in v4 [1] for same patch.
> 
> [1] http://dpdk.org/dev/patchwork/patch/27600/ 
> 

yes, I'm ok with your proposition:
- MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS and _PHYS_CONTIG are capa flags,
  not set by application but by the handler
- the help says that _BLK_ALIGNED_OBJECTS implies _PHYS_CONTIG
- test both (_BLK_ALIGNED_OBJECTS | _PHYS_CONTIG) in _xmem_size()/_usage()

Patch
diff mbox 

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 103fbd0ed..38dab1067 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 unsigned int flags)
+		      unsigned int flags)
 {
 	size_t obj_per_page, pg_num, pg_sz;
 
+	if (flags & MEMPOOL_F_BLK_ALIGNED_OBJECTS)
+		/* alignment need one additional object */
+		elt_num += 1;
+
 	if (total_elt_sz == 0)
 		return 0;
 
@@ -265,13 +269,17 @@  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 unsigned int flags)
+	uint32_t pg_shift, unsigned int flags)
 {
 	uint32_t elt_cnt = 0;
 	phys_addr_t start, end;
 	uint32_t paddr_idx;
 	size_t pg_sz = (size_t)1 << pg_shift;
 
+	if (flags & MEMPOOL_F_BLK_ALIGNED_OBJECTS)
+		/* alignment need one additional object */
+		elt_num += 1;
+
 	/* if paddr is NULL, assume contiguous memory */
 	if (paddr == NULL) {
 		start = 0;
@@ -390,7 +398,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_BLK_ALIGNED_OBJECTS)
+		/* 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 63688faff..110ffb601 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -271,6 +271,10 @@  struct rte_mempool {
  * Note: This flag should not be passed by application.
  */
 #define MEMPOOL_F_CAPA_PHYS_CONTIG 0x0040
+/**
+ * Align object start address to total elem size
+ */
+#define MEMPOOL_F_BLK_ALIGNED_OBJECTS 0x0080
 
 /**
  * @internal When debug is enabled, store some statistics.