diff mbox

[dpdk-dev,2/4] mempool: detect physical contiguous object in pool

Message ID 20170621173248.1313-3-santosh.shukla@caviumnetworks.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers

Checks

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

Commit Message

Santosh Shukla June 21, 2017, 5:32 p.m. UTC 
  HW mempool blocks may need physical contiguous obj in a pool.
Introducing MEMPOOL_F_POOL_CONTIG flag for such use-case.  The flag
useful to detect whether all buffer fits within a hugepage or not. If
not then return -ENOSPC. This way, we make sure that all object within a
pool is contiguous.

Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 lib/librte_mempool/rte_mempool.c | 8 ++++++++
 lib/librte_mempool/rte_mempool.h | 1 +
 2 files changed, 9 insertions(+)

Comments

Olivier Matz July 3, 2017, 4:37 p.m. UTC | #1 
On Wed, 21 Jun 2017 17:32:46 +0000, Santosh Shukla <santosh.shukla@caviumnetworks.com> wrote:
> HW mempool blocks may need physical contiguous obj in a pool.

This should be clarified: the memory area containing all the
objects must be physically contiguous, right?

> Introducing MEMPOOL_F_POOL_CONTIG flag for such use-case.  The flag
> useful to detect whether all buffer fits within a hugepage or not. If
> not then return -ENOSPC. This way, we make sure that all object within a
> pool is contiguous.
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
>  lib/librte_mempool/rte_mempool.c | 8 ++++++++
>  lib/librte_mempool/rte_mempool.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 045baef45..7dec2f51d 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -368,6 +368,14 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr,
>  
>  	total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
>  
> +	/* Detect nb_mbuf fit in hugepage */
> +	if (mp->flags & MEMPOOL_F_POOL_CONTIG) {
> +		if (len < total_elt_sz * mp->size) {
> +			RTE_LOG(ERR, MEMPOOL, "nb_mbufs not fitting in one hugepage,..exit\n");
> +			return -ENOSPC;
> +		}
> +	}
> +

We should not reference mbuf, we are in mempool code, dealing with
any kind of object.

Also, len is not necessarily the size of a hugepage, but the size of the
physical area passed to te_mempool_populate_phys().

>  	memhdr = rte_zmalloc("MEMPOOL_MEMHDR", sizeof(*memhdr), 0);
>  	if (memhdr == NULL)
>  		return -ENOMEM;
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index c3cdc77e4..fd8722e69 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_SC_GET         0x0008 /**< Default get is "single-consumer".*/
>  #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 */

We must highlight here that it's a capability flag.
Following my other comments on the first patch, this define should be
renamed in something else. I suggest:

#define RTE_MEMPOOL_CAPA_PHYS_CONTIG 0x0001

The description should be longer and more accurate.

I'm also a bit puzzled because this is more a limitation than a
capability.

>  
>  /**
>   * @internal When debug is enabled, store some statistics.
Santosh Shukla July 5, 2017, 7:07 a.m. UTC | #2 
Hi Olivier,

On Monday 03 July 2017 10:07 PM, Olivier Matz wrote:

> On Wed, 21 Jun 2017 17:32:46 +0000, Santosh Shukla <santosh.shukla@caviumnetworks.com> wrote:
>> HW mempool blocks may need physical contiguous obj in a pool.
> This should be clarified: the memory area containing all the
> objects must be physically contiguous, right?

ok.

>> Introducing MEMPOOL_F_POOL_CONTIG flag for such use-case.  The flag
>> useful to detect whether all buffer fits within a hugepage or not. If
>> not then return -ENOSPC. This way, we make sure that all object within a
>> pool is contiguous.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> ---
>>  lib/librte_mempool/rte_mempool.c | 8 ++++++++
>>  lib/librte_mempool/rte_mempool.h | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 045baef45..7dec2f51d 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -368,6 +368,14 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr,
>>  
>>  	total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
>>  
>> +	/* Detect nb_mbuf fit in hugepage */
>> +	if (mp->flags & MEMPOOL_F_POOL_CONTIG) {
>> +		if (len < total_elt_sz * mp->size) {
>> +			RTE_LOG(ERR, MEMPOOL, "nb_mbufs not fitting in one hugepage,..exit\n");
>> +			return -ENOSPC;
>> +		}
>> +	}
>> +
> We should not reference mbuf, we are in mempool code, dealing with
> any kind of object.

ok, in v2.

> Also, len is not necessarily the size of a hugepage, but the size of the
> physical area passed to te_mempool_populate_phys().

The idea is to make sure that blk_sz (total_elt_sz * mp->size) fits with in
hugepage. So if rte_eal_has_hugepages() is true then 'len' is
hugepage size, in-case of non-hugepage this condition would fail.

Does that make sense? if so then I'll modify comment and error log.
Otherwise could you pl. suggest better approach to detect phys contiguity.

>>  	memhdr = rte_zmalloc("MEMPOOL_MEMHDR", sizeof(*memhdr), 0);
>>  	if (memhdr == NULL)
>>  		return -ENOMEM;
>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>> index c3cdc77e4..fd8722e69 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_SC_GET         0x0008 /**< Default get is "single-consumer".*/
>>  #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 */
> We must highlight here that it's a capability flag.
> Following my other comments on the first patch, this define should be
> renamed in something else. I suggest:
>
> #define RTE_MEMPOOL_CAPA_PHYS_CONTIG 0x0001
>
> The description should be longer and more accurate.
>
> I'm also a bit puzzled because this is more a limitation than a
> capability.

ok with renaming flag but per my [1/4] comment.

But i find it makes more sense to - not differentiate PHYS_CONTIG
with existing mempool cap flags.

>>  
>>  /**
>>   * @internal When debug is enabled, store some statistics.
diff mbox

Patch

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 045baef45..7dec2f51d 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -368,6 +368,14 @@  rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr,
 
 	total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
 
+	/* Detect nb_mbuf fit in hugepage */
+	if (mp->flags & MEMPOOL_F_POOL_CONTIG) {
+		if (len < total_elt_sz * mp->size) {
+			RTE_LOG(ERR, MEMPOOL, "nb_mbufs not fitting in one hugepage,..exit\n");
+			return -ENOSPC;
+		}
+	}
+
 	memhdr = rte_zmalloc("MEMPOOL_MEMHDR", sizeof(*memhdr), 0);
 	if (memhdr == NULL)
 		return -ENOMEM;
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index c3cdc77e4..fd8722e69 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_SC_GET         0x0008 /**< Default get is "single-consumer".*/
 #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 */
 
 /**
  * @internal When debug is enabled, store some statistics.