[PATCH 1/2] eal/mempool: introduce check for external mempool availability

[PATCH 1/2] eal/mempool: introduce check for external mempool availability

[Hemant Agrawal]

External offloaded mempool may not be available always. This check enables
run time verification of the presence of external mempool before the
mempool ops are installed. 

An application built with a specific external mempool may work fine
on host. But, may not work on VM, specificaly if non-hw specific interfaces
are used e.g. virtio-net.

This is required to make sure that same application can work in all
environment for a given hw platform.

The solution proposed here is that rte_mempool_set_ops_byname should return
specific error in case the current execution environment is not supporting
the given mempool. Thereby allowing it to fallback on software mempool
implementation e.g. "ring_mp_mc"

This patch introduces new optional "pool_verify" as mempool ops function
to check if external mempool instance is available or not.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 lib/librte_mempool/rte_mempool.h       | 21 +++++++++++++++++++++
 lib/librte_mempool/rte_mempool_ops.c   | 18 ++++++++++++++++++
 lib/librte_mempool/rte_mempool_ring.c  |  4 ++++
 lib/librte_mempool/rte_mempool_stack.c |  3 ++-
 4 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 059ad9e..67e12d1 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -385,6 +385,12 @@ typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp,
  */
 typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool *mp);
 
+/**
+ * Return if the given external mempool is available for this instance.
+ * it is optional to implement for mempools
+ */
+typedef int (*rte_mempool_pool_verify)(const struct rte_mempool *mp);
+
 /** Structure defining mempool operations structure */
 struct rte_mempool_ops {
 	char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
@@ -393,6 +399,8 @@ struct rte_mempool_ops {
 	rte_mempool_enqueue_t enqueue;   /**< Enqueue an object. */
 	rte_mempool_dequeue_t dequeue;   /**< Dequeue an object. */
 	rte_mempool_get_count get_count; /**< Get qty of available objs. */
+	rte_mempool_pool_verify pool_verify;
+	/**< Verify if external mempool is available for usages*/
 } __rte_cache_aligned;
 
 #define RTE_MEMPOOL_MAX_OPS_IDX 16  /**< Max registered ops structs */
@@ -514,6 +522,18 @@ void
 rte_mempool_ops_free(struct rte_mempool *mp);
 
 /**
+ * @internal wrapper to verify the external mempool availability
+ *
+ * @param mp
+ *   Pointer to the memory pool.
+ * @return
+ *   0: Success; external mempool instance is available
+ * - <0: Error; external mempool instance is not available
+ */
+int
+rte_mempool_ops_pool_verify(const struct rte_mempool *mp);
+
+/**
  * Set the ops of a mempool.
  *
  * This can only be done on a mempool that is not populated, i.e. just after
@@ -529,6 +549,7 @@ rte_mempool_ops_free(struct rte_mempool *mp);
  *   - 0: Success; the mempool is now using the requested ops functions.
  *   - -EINVAL - Invalid ops struct name provided.
  *   - -EEXIST - mempool already has an ops struct assigned.
+ *   - -EOPNOTSUPP  - mempool instance not available.
  */
 int
 rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index 5f24de2..4119882 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -123,6 +123,18 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
 	return ops->get_count(mp);
 }
 
+/* wrapper to check if given external mempool is available for this instance.*/
+int
+rte_mempool_ops_pool_verify(const struct rte_mempool *mp)
+{
+	struct rte_mempool_ops *ops;
+
+	ops = rte_mempool_get_ops(mp->ops_index);
+	if (ops->pool_verify)
+		return ops->pool_verify(mp);
+	return 0;
+}
+
 /* sets mempool ops previously registered by rte_mempool_register_ops. */
 int
 rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
@@ -146,6 +158,12 @@ rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
 	if (ops == NULL)
 		return -EINVAL;
 
+	/*verify if the given mempool is available for this instance */
+	if (ops->pool_verify) {
+		if (ops->pool_verify(mp))
+			return -EOPNOTSUPP;
+	}
+
 	mp->ops_index = i;
 	mp->pool_config = pool_config;
 	return 0;
diff --git a/lib/librte_mempool/rte_mempool_ring.c b/lib/librte_mempool/rte_mempool_ring.c
index b9aa64d..d86d5e0 100644
--- a/lib/librte_mempool/rte_mempool_ring.c
+++ b/lib/librte_mempool/rte_mempool_ring.c
@@ -126,6 +126,7 @@ static const struct rte_mempool_ops ops_mp_mc = {
 	.enqueue = common_ring_mp_enqueue,
 	.dequeue = common_ring_mc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 static const struct rte_mempool_ops ops_sp_sc = {
@@ -135,6 +136,7 @@ static const struct rte_mempool_ops ops_sp_sc = {
 	.enqueue = common_ring_sp_enqueue,
 	.dequeue = common_ring_sc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 static const struct rte_mempool_ops ops_mp_sc = {
@@ -144,6 +146,7 @@ static const struct rte_mempool_ops ops_mp_sc = {
 	.enqueue = common_ring_mp_enqueue,
 	.dequeue = common_ring_sc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 static const struct rte_mempool_ops ops_sp_mc = {
@@ -153,6 +156,7 @@ static const struct rte_mempool_ops ops_sp_mc = {
 	.enqueue = common_ring_sp_enqueue,
 	.dequeue = common_ring_mc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 MEMPOOL_REGISTER_OPS(ops_mp_mc);
diff --git a/lib/librte_mempool/rte_mempool_stack.c b/lib/librte_mempool/rte_mempool_stack.c
index 5fd8af2..0198b70 100644
--- a/lib/librte_mempool/rte_mempool_stack.c
+++ b/lib/librte_mempool/rte_mempool_stack.c
@@ -141,7 +141,8 @@ static struct rte_mempool_ops ops_stack = {
 	.free = stack_free,
 	.enqueue = stack_enqueue,
 	.dequeue = stack_dequeue,
-	.get_count = stack_get_count
+	.get_count = stack_get_count,
+	.pool_verify = NULL,
 };
 
 MEMPOOL_REGISTER_OPS(ops_stack);
-- 
1.9.1

[PATCH 2/2] mempool:pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

In the rte_pktmbuf_pool_create, if the default external mempool is
not available, the implementation can default to "ring_mp_mc", which
is an software implementation.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 lib/librte_mbuf/rte_mbuf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 4846b89..4adb4f5 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -176,6 +176,11 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
 
 	rte_errno = rte_mempool_set_ops_byname(mp,
 			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
+
+	/* on error, try falling back to the software based default pool */
+	if (rte_errno == -EOPNOTSUPP)
+		rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);
+
 	if (rte_errno != 0) {
 		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
 		return NULL;
-- 
1.9.1

[PATCH v2 1/2] eal/mempool: introduce check for external mempool availability

[Hemant Agrawal]

External offloaded mempool may not be available always. This check enables
run time verification of the presence of external mempool before the
mempool ops are installed.

An application built with a specific external mempool may work fine
on host. But, may not work on VM, specificaly if non-hw specific interfaces
are used e.g. virtio-net.

This is required to make sure that same application can work in all
environment for a given hw platform.

The solution proposed here is that rte_mempool_set_ops_byname should return
specific error in case the current execution environment is not supporting
the given mempool. Thereby allowing it to fallback on software mempool
implementation e.g. "ring_mp_mc"

This patch introduces new optional "pool_verify" as mempool ops function
to check if external mempool instance is available or not.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
Changes in v2:
* fixed the pool_verify copy in rte_mempool_register_ops
---
 lib/librte_mempool/rte_mempool.h       | 21 +++++++++++++++++++++
 lib/librte_mempool/rte_mempool_ops.c   | 19 +++++++++++++++++++
 lib/librte_mempool/rte_mempool_ring.c  |  4 ++++
 lib/librte_mempool/rte_mempool_stack.c |  3 ++-
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 0243f9e..8599df1 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -387,6 +387,12 @@ typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp,
  */
 typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool *mp);
 
+/**
+ * Return if the given external mempool is available for this instance.
+ * it is optional to implement for mempools
+ */
+typedef int (*rte_mempool_pool_verify)(const struct rte_mempool *mp);
+
 /** Structure defining mempool operations structure */
 struct rte_mempool_ops {
 	char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
@@ -395,6 +401,8 @@ struct rte_mempool_ops {
 	rte_mempool_enqueue_t enqueue;   /**< Enqueue an object. */
 	rte_mempool_dequeue_t dequeue;   /**< Dequeue an object. */
 	rte_mempool_get_count get_count; /**< Get qty of available objs. */
+	rte_mempool_pool_verify pool_verify;
+	/**< Verify if external mempool is available for usages*/
 } __rte_cache_aligned;
 
 #define RTE_MEMPOOL_MAX_OPS_IDX 16  /**< Max registered ops structs */
@@ -516,6 +524,18 @@ void
 rte_mempool_ops_free(struct rte_mempool *mp);
 
 /**
+ * @internal wrapper to verify the external mempool availability
+ *
+ * @param mp
+ *   Pointer to the memory pool.
+ * @return
+ *   0: Success; external mempool instance is available
+ * - <0: Error; external mempool instance is not available
+ */
+int
+rte_mempool_ops_pool_verify(const struct rte_mempool *mp);
+
+/**
  * Set the ops of a mempool.
  *
  * This can only be done on a mempool that is not populated, i.e. just after
@@ -531,6 +551,7 @@ rte_mempool_ops_free(struct rte_mempool *mp);
  *   - 0: Success; the mempool is now using the requested ops functions.
  *   - -EINVAL - Invalid ops struct name provided.
  *   - -EEXIST - mempool already has an ops struct assigned.
+ *   - -EOPNOTSUPP  - mempool instance not available.
  */
 int
 rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index 5f24de2..c2e765f 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
 	ops->enqueue = h->enqueue;
 	ops->dequeue = h->dequeue;
 	ops->get_count = h->get_count;
+	ops->pool_verify = h->pool_verify;
 
 	rte_spinlock_unlock(&rte_mempool_ops_table.sl);
 
@@ -123,6 +124,18 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
 	return ops->get_count(mp);
 }
 
+/* wrapper to check if given external mempool is available for this instance.*/
+int
+rte_mempool_ops_pool_verify(const struct rte_mempool *mp)
+{
+	struct rte_mempool_ops *ops;
+
+	ops = rte_mempool_get_ops(mp->ops_index);
+	if (ops->pool_verify)
+		return ops->pool_verify(mp);
+	return 0;
+}
+
 /* sets mempool ops previously registered by rte_mempool_register_ops. */
 int
 rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
@@ -146,6 +159,12 @@ rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
 	if (ops == NULL)
 		return -EINVAL;
 
+	/*verify if the given mempool is available for this instance */
+	if (ops->pool_verify) {
+		if (ops->pool_verify(mp))
+			return -EOPNOTSUPP;
+	}
+
 	mp->ops_index = i;
 	mp->pool_config = pool_config;
 	return 0;
diff --git a/lib/librte_mempool/rte_mempool_ring.c b/lib/librte_mempool/rte_mempool_ring.c
index b9aa64d..d86d5e0 100644
--- a/lib/librte_mempool/rte_mempool_ring.c
+++ b/lib/librte_mempool/rte_mempool_ring.c
@@ -126,6 +126,7 @@ static const struct rte_mempool_ops ops_mp_mc = {
 	.enqueue = common_ring_mp_enqueue,
 	.dequeue = common_ring_mc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 static const struct rte_mempool_ops ops_sp_sc = {
@@ -135,6 +136,7 @@ static const struct rte_mempool_ops ops_sp_sc = {
 	.enqueue = common_ring_sp_enqueue,
 	.dequeue = common_ring_sc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 static const struct rte_mempool_ops ops_mp_sc = {
@@ -144,6 +146,7 @@ static const struct rte_mempool_ops ops_mp_sc = {
 	.enqueue = common_ring_mp_enqueue,
 	.dequeue = common_ring_sc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 static const struct rte_mempool_ops ops_sp_mc = {
@@ -153,6 +156,7 @@ static const struct rte_mempool_ops ops_sp_mc = {
 	.enqueue = common_ring_sp_enqueue,
 	.dequeue = common_ring_mc_dequeue,
 	.get_count = common_ring_get_count,
+	.pool_verify = NULL,
 };
 
 MEMPOOL_REGISTER_OPS(ops_mp_mc);
diff --git a/lib/librte_mempool/rte_mempool_stack.c b/lib/librte_mempool/rte_mempool_stack.c
index 5fd8af2..0198b70 100644
--- a/lib/librte_mempool/rte_mempool_stack.c
+++ b/lib/librte_mempool/rte_mempool_stack.c
@@ -141,7 +141,8 @@ static struct rte_mempool_ops ops_stack = {
 	.free = stack_free,
 	.enqueue = stack_enqueue,
 	.dequeue = stack_dequeue,
-	.get_count = stack_get_count
+	.get_count = stack_get_count,
+	.pool_verify = NULL,
 };
 
 MEMPOOL_REGISTER_OPS(ops_stack);
-- 
1.9.1

[PATCH v2 2/2] mempool:pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

In the rte_pktmbuf_pool_create, if the default external mempool is
not available, the implementation can default to "ring_mp_mc", which
is an software implementation.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 lib/librte_mbuf/rte_mbuf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 4846b89..4adb4f5 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -176,6 +176,11 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
 
 	rte_errno = rte_mempool_set_ops_byname(mp,
 			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
+
+	/* on error, try falling back to the software based default pool */
+	if (rte_errno == -EOPNOTSUPP)
+		rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);
+
 	if (rte_errno != 0) {
 		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
 		return NULL;
-- 
1.9.1

Re: [PATCH v2 2/2] mempool:pktmbuf pool default fallback for mempool ops error

[Hunt, David]

Hi Hemant,

On 15/9/2016 6:13 PM, Hemant Agrawal wrote:
> In the rte_pktmbuf_pool_create, if the default external mempool is
> not available, the implementation can default to "ring_mp_mc", which
> is an software implementation.
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>   lib/librte_mbuf/rte_mbuf.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 4846b89..4adb4f5 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -176,6 +176,11 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
>   
>   	rte_errno = rte_mempool_set_ops_byname(mp,
>   			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
> +
> +	/* on error, try falling back to the software based default pool */
> +	if (rte_errno == -EOPNOTSUPP)
> +		rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);

Should we log a warning message here saying that we're falling back to 
the mp/mc handler?

> +
>   	if (rte_errno != 0) {
>   		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>   		return NULL;

Re: [PATCH v2 1/2] eal/mempool: introduce check for external mempool availability

[Hunt, David]

Hi Hemant,

On 15/9/2016 6:13 PM, Hemant Agrawal wrote:
> External offloaded mempool may not be available always. This check enables
> run time verification of the presence of external mempool before the
> mempool ops are installed.
>
> An application built with a specific external mempool may work fine
> on host. But, may not work on VM, specificaly if non-hw specific interfaces
> are used e.g. virtio-net.
>
> This is required to make sure that same application can work in all
> environment for a given hw platform.
>
> The solution proposed here is that rte_mempool_set_ops_byname should return
> specific error in case the current execution environment is not supporting
> the given mempool. Thereby allowing it to fallback on software mempool
> implementation e.g. "ring_mp_mc"
>
> This patch introduces new optional "pool_verify" as mempool ops function
> to check if external mempool instance is available or not.

I've a small suggestion around the naming of the new functions. It seems 
to me that this patch is more about mempool handler 'support' than 
'verification'.
Could I suggest a different name for this new function? I think maybe 
"supported" may be more appropriate than "pool_verify". The return code 
that's returned when a mempool handler is not available is -EOPNOTSUPP, 
which is what suggested the word "supported" to me. I think that:

	if (ops->supported)
		return ops->supported(mp);

makes for slightly easier reading. The wrapper function would logically 
then be called  rte_mempool_ops_supported().


> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> Changes in v2:
> * fixed the pool_verify copy in rte_mempool_register_ops
> ---
>   lib/librte_mempool/rte_mempool.h       | 21 +++++++++++++++++++++
>   lib/librte_mempool/rte_mempool_ops.c   | 19 +++++++++++++++++++
>   lib/librte_mempool/rte_mempool_ring.c  |  4 ++++
>   lib/librte_mempool/rte_mempool_stack.c |  3 ++-
>   4 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 0243f9e..8599df1 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -387,6 +387,12 @@ typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp,
>    */
>   typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool *mp);
>   
> +/**
> + * Return if the given external mempool is available for this instance.
> + * it is optional to implement for mempools
> + */
> +typedef int (*rte_mempool_pool_verify)(const struct rte_mempool *mp);
> +
>   /** Structure defining mempool operations structure */
>   struct rte_mempool_ops {
>   	char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
> @@ -395,6 +401,8 @@ struct rte_mempool_ops {
>   	rte_mempool_enqueue_t enqueue;   /**< Enqueue an object. */
>   	rte_mempool_dequeue_t dequeue;   /**< Dequeue an object. */
>   	rte_mempool_get_count get_count; /**< Get qty of available objs. */
> +	rte_mempool_pool_verify pool_verify;
> +	/**< Verify if external mempool is available for usages*/
>   } __rte_cache_aligned;
>   
>   #define RTE_MEMPOOL_MAX_OPS_IDX 16  /**< Max registered ops structs */
> @@ -516,6 +524,18 @@ void
>   rte_mempool_ops_free(struct rte_mempool *mp);
>   
>   /**
> + * @internal wrapper to verify the external mempool availability
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @return
> + *   0: Success; external mempool instance is available
> + * - <0: Error; external mempool instance is not available
> + */
> +int
> +rte_mempool_ops_pool_verify(const struct rte_mempool *mp);
> +
> +/**
>    * Set the ops of a mempool.
>    *
>    * This can only be done on a mempool that is not populated, i.e. just after
> @@ -531,6 +551,7 @@ rte_mempool_ops_free(struct rte_mempool *mp);
>    *   - 0: Success; the mempool is now using the requested ops functions.
>    *   - -EINVAL - Invalid ops struct name provided.
>    *   - -EEXIST - mempool already has an ops struct assigned.
> + *   - -EOPNOTSUPP  - mempool instance not available.
>    */
>   int
>   rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
> diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> index 5f24de2..c2e765f 100644
> --- a/lib/librte_mempool/rte_mempool_ops.c
> +++ b/lib/librte_mempool/rte_mempool_ops.c
> @@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
>   	ops->enqueue = h->enqueue;
>   	ops->dequeue = h->dequeue;
>   	ops->get_count = h->get_count;
> +	ops->pool_verify = h->pool_verify;
>   
>   	rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>   
> @@ -123,6 +124,18 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
>   	return ops->get_count(mp);
>   }
>   
> +/* wrapper to check if given external mempool is available for this instance.*/
> +int
> +rte_mempool_ops_pool_verify(const struct rte_mempool *mp)
> +{
> +	struct rte_mempool_ops *ops;
> +
> +	ops = rte_mempool_get_ops(mp->ops_index);
> +	if (ops->pool_verify)
> +		return ops->pool_verify(mp);
> +	return 0;
> +}
> +
>   /* sets mempool ops previously registered by rte_mempool_register_ops. */
>   int
>   rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
> @@ -146,6 +159,12 @@ rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
>   	if (ops == NULL)
>   		return -EINVAL;
>   
> +	/*verify if the given mempool is available for this instance */
> +	if (ops->pool_verify) {
> +		if (ops->pool_verify(mp))
> +			return -EOPNOTSUPP;
> +	}
> +
>   	mp->ops_index = i;
>   	mp->pool_config = pool_config;
>   	return 0;
> diff --git a/lib/librte_mempool/rte_mempool_ring.c b/lib/librte_mempool/rte_mempool_ring.c
> index b9aa64d..d86d5e0 100644
> --- a/lib/librte_mempool/rte_mempool_ring.c
> +++ b/lib/librte_mempool/rte_mempool_ring.c
> @@ -126,6 +126,7 @@ static const struct rte_mempool_ops ops_mp_mc = {
>   	.enqueue = common_ring_mp_enqueue,
>   	.dequeue = common_ring_mc_dequeue,
>   	.get_count = common_ring_get_count,
> +	.pool_verify = NULL,
>   };
>   
>   static const struct rte_mempool_ops ops_sp_sc = {
> @@ -135,6 +136,7 @@ static const struct rte_mempool_ops ops_sp_sc = {
>   	.enqueue = common_ring_sp_enqueue,
>   	.dequeue = common_ring_sc_dequeue,
>   	.get_count = common_ring_get_count,
> +	.pool_verify = NULL,
>   };
>   
>   static const struct rte_mempool_ops ops_mp_sc = {
> @@ -144,6 +146,7 @@ static const struct rte_mempool_ops ops_mp_sc = {
>   	.enqueue = common_ring_mp_enqueue,
>   	.dequeue = common_ring_sc_dequeue,
>   	.get_count = common_ring_get_count,
> +	.pool_verify = NULL,
>   };
>   
>   static const struct rte_mempool_ops ops_sp_mc = {
> @@ -153,6 +156,7 @@ static const struct rte_mempool_ops ops_sp_mc = {
>   	.enqueue = common_ring_sp_enqueue,
>   	.dequeue = common_ring_mc_dequeue,
>   	.get_count = common_ring_get_count,
> +	.pool_verify = NULL,
>   };
>   
>   MEMPOOL_REGISTER_OPS(ops_mp_mc);
> diff --git a/lib/librte_mempool/rte_mempool_stack.c b/lib/librte_mempool/rte_mempool_stack.c
> index 5fd8af2..0198b70 100644
> --- a/lib/librte_mempool/rte_mempool_stack.c
> +++ b/lib/librte_mempool/rte_mempool_stack.c
> @@ -141,7 +141,8 @@ static struct rte_mempool_ops ops_stack = {
>   	.free = stack_free,
>   	.enqueue = stack_enqueue,
>   	.dequeue = stack_dequeue,
> -	.get_count = stack_get_count
> +	.get_count = stack_get_count,
> +	.pool_verify = NULL,
>   };
>   
>   MEMPOOL_REGISTER_OPS(ops_stack);

Regards,
Dave.

Re: [PATCH v2 1/2] eal/mempool: introduce check for external mempool availability

[Hemant Agrawal]

HI David,

> -----Original Message-----
> From: Hunt, David [mailto:david.hunt@intel.com]
> Sent: Friday, September 16, 2016 2:44 PM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>; olivier.matz@6wind.com
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com
> Subject: Re: [PATCH v2 1/2] eal/mempool: introduce check for external
> mempool availability
> 
> Hi Hemant,
> 
> On 15/9/2016 6:13 PM, Hemant Agrawal wrote:
> > External offloaded mempool may not be available always. This check
> > enables run time verification of the presence of external mempool
> > before the mempool ops are installed.
> >
> > An application built with a specific external mempool may work fine on
> > host. But, may not work on VM, specificaly if non-hw specific
> > interfaces are used e.g. virtio-net.
> >
> > This is required to make sure that same application can work in all
> > environment for a given hw platform.
> >
> > The solution proposed here is that rte_mempool_set_ops_byname should
> > return specific error in case the current execution environment is not
> > supporting the given mempool. Thereby allowing it to fallback on
> > software mempool implementation e.g. "ring_mp_mc"
> >
> > This patch introduces new optional "pool_verify" as mempool ops
> > function to check if external mempool instance is available or not.
> 
> I've a small suggestion around the naming of the new functions. It seems to me
> that this patch is more about mempool handler 'support' than 'verification'.
> Could I suggest a different name for this new function? I think maybe
> "supported" may be more appropriate than "pool_verify". The return code that's
> returned when a mempool handler is not available is -EOPNOTSUPP, which is
> what suggested the word "supported" to me. I think that:
> 
> 	if (ops->supported)
> 		return ops->supported(mp);
> 
> makes for slightly easier reading. The wrapper function would logically then be
> called  rte_mempool_ops_supported().

[Hemant] Good suggestion. I will update it in next version. 

> 
> 
> > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > ---
> > Changes in v2:
> > * fixed the pool_verify copy in rte_mempool_register_ops
> > ---
> >   lib/librte_mempool/rte_mempool.h       | 21 +++++++++++++++++++++
> >   lib/librte_mempool/rte_mempool_ops.c   | 19 +++++++++++++++++++
> >   lib/librte_mempool/rte_mempool_ring.c  |  4 ++++
> >   lib/librte_mempool/rte_mempool_stack.c |  3 ++-
> >   4 files changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_mempool/rte_mempool.h
> b/lib/librte_mempool/rte_mempool.h
> > index 0243f9e..8599df1 100644
> > --- a/lib/librte_mempool/rte_mempool.h
> > +++ b/lib/librte_mempool/rte_mempool.h
> > @@ -387,6 +387,12 @@ typedef int (*rte_mempool_dequeue_t)(struct
> rte_mempool *mp,
> >    */
> >   typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool
> *mp);
> >
> > +/**
> > + * Return if the given external mempool is available for this instance.
> > + * it is optional to implement for mempools
> > + */
> > +typedef int (*rte_mempool_pool_verify)(const struct rte_mempool *mp);
> > +
> >   /** Structure defining mempool operations structure */
> >   struct rte_mempool_ops {
> >   	char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool
> ops struct. */
> > @@ -395,6 +401,8 @@ struct rte_mempool_ops {
> >   	rte_mempool_enqueue_t enqueue;   /**< Enqueue an object. */
> >   	rte_mempool_dequeue_t dequeue;   /**< Dequeue an object. */
> >   	rte_mempool_get_count get_count; /**< Get qty of available objs. */
> > +	rte_mempool_pool_verify pool_verify;
> > +	/**< Verify if external mempool is available for usages*/
> >   } __rte_cache_aligned;
> >
> >   #define RTE_MEMPOOL_MAX_OPS_IDX 16  /**< Max registered ops structs
> */
> > @@ -516,6 +524,18 @@ void
> >   rte_mempool_ops_free(struct rte_mempool *mp);
> >
> >   /**
> > + * @internal wrapper to verify the external mempool availability
> > + *
> > + * @param mp
> > + *   Pointer to the memory pool.
> > + * @return
> > + *   0: Success; external mempool instance is available
> > + * - <0: Error; external mempool instance is not available
> > + */
> > +int
> > +rte_mempool_ops_pool_verify(const struct rte_mempool *mp);
> > +
> > +/**
> >    * Set the ops of a mempool.
> >    *
> >    * This can only be done on a mempool that is not populated, i.e. just after
> > @@ -531,6 +551,7 @@ rte_mempool_ops_free(struct rte_mempool *mp);
> >    *   - 0: Success; the mempool is now using the requested ops functions.
> >    *   - -EINVAL - Invalid ops struct name provided.
> >    *   - -EEXIST - mempool already has an ops struct assigned.
> > + *   - -EOPNOTSUPP  - mempool instance not available.
> >    */
> >   int
> >   rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
> > diff --git a/lib/librte_mempool/rte_mempool_ops.c
> b/lib/librte_mempool/rte_mempool_ops.c
> > index 5f24de2..c2e765f 100644
> > --- a/lib/librte_mempool/rte_mempool_ops.c
> > +++ b/lib/librte_mempool/rte_mempool_ops.c
> > @@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct
> rte_mempool_ops *h)
> >   	ops->enqueue = h->enqueue;
> >   	ops->dequeue = h->dequeue;
> >   	ops->get_count = h->get_count;
> > +	ops->pool_verify = h->pool_verify;
> >
> >   	rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> >
> > @@ -123,6 +124,18 @@ rte_mempool_ops_get_count(const struct
> rte_mempool *mp)
> >   	return ops->get_count(mp);
> >   }
> >
> > +/* wrapper to check if given external mempool is available for this
> instance.*/
> > +int
> > +rte_mempool_ops_pool_verify(const struct rte_mempool *mp)
> > +{
> > +	struct rte_mempool_ops *ops;
> > +
> > +	ops = rte_mempool_get_ops(mp->ops_index);
> > +	if (ops->pool_verify)
> > +		return ops->pool_verify(mp);
> > +	return 0;
> > +}
> > +
> >   /* sets mempool ops previously registered by rte_mempool_register_ops. */
> >   int
> >   rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
> > @@ -146,6 +159,12 @@ rte_mempool_set_ops_byname(struct rte_mempool
> *mp, const char *name,
> >   	if (ops == NULL)
> >   		return -EINVAL;
> >
> > +	/*verify if the given mempool is available for this instance */
> > +	if (ops->pool_verify) {
> > +		if (ops->pool_verify(mp))
> > +			return -EOPNOTSUPP;
> > +	}
> > +
> >   	mp->ops_index = i;
> >   	mp->pool_config = pool_config;
> >   	return 0;
> > diff --git a/lib/librte_mempool/rte_mempool_ring.c
> b/lib/librte_mempool/rte_mempool_ring.c
> > index b9aa64d..d86d5e0 100644
> > --- a/lib/librte_mempool/rte_mempool_ring.c
> > +++ b/lib/librte_mempool/rte_mempool_ring.c
> > @@ -126,6 +126,7 @@ static const struct rte_mempool_ops ops_mp_mc = {
> >   	.enqueue = common_ring_mp_enqueue,
> >   	.dequeue = common_ring_mc_dequeue,
> >   	.get_count = common_ring_get_count,
> > +	.pool_verify = NULL,
> >   };
> >
> >   static const struct rte_mempool_ops ops_sp_sc = {
> > @@ -135,6 +136,7 @@ static const struct rte_mempool_ops ops_sp_sc = {
> >   	.enqueue = common_ring_sp_enqueue,
> >   	.dequeue = common_ring_sc_dequeue,
> >   	.get_count = common_ring_get_count,
> > +	.pool_verify = NULL,
> >   };
> >
> >   static const struct rte_mempool_ops ops_mp_sc = {
> > @@ -144,6 +146,7 @@ static const struct rte_mempool_ops ops_mp_sc = {
> >   	.enqueue = common_ring_mp_enqueue,
> >   	.dequeue = common_ring_sc_dequeue,
> >   	.get_count = common_ring_get_count,
> > +	.pool_verify = NULL,
> >   };
> >
> >   static const struct rte_mempool_ops ops_sp_mc = {
> > @@ -153,6 +156,7 @@ static const struct rte_mempool_ops ops_sp_mc = {
> >   	.enqueue = common_ring_sp_enqueue,
> >   	.dequeue = common_ring_mc_dequeue,
> >   	.get_count = common_ring_get_count,
> > +	.pool_verify = NULL,
> >   };
> >
> >   MEMPOOL_REGISTER_OPS(ops_mp_mc);
> > diff --git a/lib/librte_mempool/rte_mempool_stack.c
> b/lib/librte_mempool/rte_mempool_stack.c
> > index 5fd8af2..0198b70 100644
> > --- a/lib/librte_mempool/rte_mempool_stack.c
> > +++ b/lib/librte_mempool/rte_mempool_stack.c
> > @@ -141,7 +141,8 @@ static struct rte_mempool_ops ops_stack = {
> >   	.free = stack_free,
> >   	.enqueue = stack_enqueue,
> >   	.dequeue = stack_dequeue,
> > -	.get_count = stack_get_count
> > +	.get_count = stack_get_count,
> > +	.pool_verify = NULL,
> >   };
> >
> >   MEMPOOL_REGISTER_OPS(ops_stack);
> 
> Regards,
> Dave.

Re: [PATCH v2 2/2] mempool:pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

Hi David,

> -----Original Message-----
> From: Hunt, David [mailto:david.hunt@intel.com]
> Sent: Friday, September 16, 2016 2:00 PM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>; olivier.matz@6wind.com
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com
> Subject: Re: [PATCH v2 2/2] mempool:pktmbuf pool default fallback for
> mempool ops error
> 
> Hi Hemant,
> 
> On 15/9/2016 6:13 PM, Hemant Agrawal wrote:
> > In the rte_pktmbuf_pool_create, if the default external mempool is not
> > available, the implementation can default to "ring_mp_mc", which is an
> > software implementation.
> >
> > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > ---
> >   lib/librte_mbuf/rte_mbuf.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 4846b89..4adb4f5 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -176,6 +176,11 @@ rte_pktmbuf_pool_create(const char *name,
> > unsigned n,
> >
> >   	rte_errno = rte_mempool_set_ops_byname(mp,
> >   			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
> > +
> > +	/* on error, try falling back to the software based default pool */
> > +	if (rte_errno == -EOPNOTSUPP)
> > +		rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc",
> NULL);
> 
> Should we log a warning message here saying that we're falling back to the
> mp/mc handler?
> 
[Hemant]Agree. Will add it. 

> > +
> >   	if (rte_errno != 0) {
> >   		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> >   		return NULL;

[PATCH v3 1/2] eal/mempool: check for external mempool support

[Hemant Agrawal]

External offloaded mempool may not be available always. This check enables
run time verification of the presence of external mempool before the
mempool ops are installed.

An application built with a specific external mempool may work fine
on host. But, may not work on VM, specificaly if non-hw specific interfaces
are used e.g. virtio-net.

This is required to make sure that same application can work in all
environment for a given hw platform.

The solution proposed here is that rte_mempool_set_ops_byname should return
specific error in case the current execution environment is not supporting
the given mempool. Thereby allowing it to fallback on software mempool
implementation e.g. "ring_mp_mc"

This patch introduces new optional "supported" as mempool ops function
to check if external mempool instance is available or not.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
Changes in v2:
* fixed the pool_verify copy in rte_mempool_register_ops

Changes in v3:
* rename the pool_verify to supported as per David's review comment.
---
 lib/librte_mempool/rte_mempool.h       | 21 +++++++++++++++++++++
 lib/librte_mempool/rte_mempool_ops.c   | 19 +++++++++++++++++++
 lib/librte_mempool/rte_mempool_ring.c  |  4 ++++
 lib/librte_mempool/rte_mempool_stack.c |  3 ++-
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 0243f9e..322f177 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -387,6 +387,12 @@ typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp,
  */
 typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool *mp);
 
+/**
+ * Return if the given external mempool is supported for this instance.
+ * it is optional to implement for mempools
+ */
+typedef int (*rte_mempool_supported)(const struct rte_mempool *mp);
+
 /** Structure defining mempool operations structure */
 struct rte_mempool_ops {
 	char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
@@ -395,6 +401,8 @@ struct rte_mempool_ops {
 	rte_mempool_enqueue_t enqueue;   /**< Enqueue an object. */
 	rte_mempool_dequeue_t dequeue;   /**< Dequeue an object. */
 	rte_mempool_get_count get_count; /**< Get qty of available objs. */
+	rte_mempool_supported supported;
+	/**< Verify if external mempool is supported for usages*/
 } __rte_cache_aligned;
 
 #define RTE_MEMPOOL_MAX_OPS_IDX 16  /**< Max registered ops structs */
@@ -516,6 +524,18 @@ void
 rte_mempool_ops_free(struct rte_mempool *mp);
 
 /**
+ * @internal wrapper to verify the external mempool availability
+ *
+ * @param mp
+ *   Pointer to the memory pool.
+ * @return
+ *   0: Success; external mempool instance is supported
+ * - <0: Error; external mempool instance is not supported
+ */
+int
+rte_mempool_ops_supported(const struct rte_mempool *mp);
+
+/**
  * Set the ops of a mempool.
  *
  * This can only be done on a mempool that is not populated, i.e. just after
@@ -531,6 +551,7 @@ rte_mempool_ops_free(struct rte_mempool *mp);
  *   - 0: Success; the mempool is now using the requested ops functions.
  *   - -EINVAL - Invalid ops struct name provided.
  *   - -EEXIST - mempool already has an ops struct assigned.
+ *   - -EOPNOTSUPP  - mempool instance not supported.
  */
 int
 rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index 5f24de2..4554062 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
 	ops->enqueue = h->enqueue;
 	ops->dequeue = h->dequeue;
 	ops->get_count = h->get_count;
+	ops->supported = h->supported;
 
 	rte_spinlock_unlock(&rte_mempool_ops_table.sl);
 
@@ -123,6 +124,18 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
 	return ops->get_count(mp);
 }
 
+/* wrapper to check if given external mempool is supported for this instance.*/
+int
+rte_mempool_ops_supported(const struct rte_mempool *mp)
+{
+	struct rte_mempool_ops *ops;
+
+	ops = rte_mempool_get_ops(mp->ops_index);
+	if (ops->supported)
+		return ops->supported(mp);
+	return 0;
+}
+
 /* sets mempool ops previously registered by rte_mempool_register_ops. */
 int
 rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
@@ -146,6 +159,12 @@ rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
 	if (ops == NULL)
 		return -EINVAL;
 
+	/*verify if the given mempool is supported for this instance */
+	if (ops->supported) {
+		if (ops->supported(mp))
+			return -EOPNOTSUPP;
+	}
+
 	mp->ops_index = i;
 	mp->pool_config = pool_config;
 	return 0;
diff --git a/lib/librte_mempool/rte_mempool_ring.c b/lib/librte_mempool/rte_mempool_ring.c
index b9aa64d..fc3db30 100644
--- a/lib/librte_mempool/rte_mempool_ring.c
+++ b/lib/librte_mempool/rte_mempool_ring.c
@@ -126,6 +126,7 @@ static const struct rte_mempool_ops ops_mp_mc = {
 	.enqueue = common_ring_mp_enqueue,
 	.dequeue = common_ring_mc_dequeue,
 	.get_count = common_ring_get_count,
+	.supported = NULL,
 };
 
 static const struct rte_mempool_ops ops_sp_sc = {
@@ -135,6 +136,7 @@ static const struct rte_mempool_ops ops_sp_sc = {
 	.enqueue = common_ring_sp_enqueue,
 	.dequeue = common_ring_sc_dequeue,
 	.get_count = common_ring_get_count,
+	.supported = NULL,
 };
 
 static const struct rte_mempool_ops ops_mp_sc = {
@@ -144,6 +146,7 @@ static const struct rte_mempool_ops ops_mp_sc = {
 	.enqueue = common_ring_mp_enqueue,
 	.dequeue = common_ring_sc_dequeue,
 	.get_count = common_ring_get_count,
+	.supported = NULL,
 };
 
 static const struct rte_mempool_ops ops_sp_mc = {
@@ -153,6 +156,7 @@ static const struct rte_mempool_ops ops_sp_mc = {
 	.enqueue = common_ring_sp_enqueue,
 	.dequeue = common_ring_mc_dequeue,
 	.get_count = common_ring_get_count,
+	.supported = NULL,
 };
 
 MEMPOOL_REGISTER_OPS(ops_mp_mc);
diff --git a/lib/librte_mempool/rte_mempool_stack.c b/lib/librte_mempool/rte_mempool_stack.c
index 5fd8af2..42ccb89 100644
--- a/lib/librte_mempool/rte_mempool_stack.c
+++ b/lib/librte_mempool/rte_mempool_stack.c
@@ -141,7 +141,8 @@ static struct rte_mempool_ops ops_stack = {
 	.free = stack_free,
 	.enqueue = stack_enqueue,
 	.dequeue = stack_dequeue,
-	.get_count = stack_get_count
+	.get_count = stack_get_count,
+	.supported = NULL,
 };
 
 MEMPOOL_REGISTER_OPS(ops_stack);
-- 
1.9.1

[PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

In the rte_pktmbuf_pool_create, if the default external mempool is
not available, the implementation can default to "ring_mp_mc", which
is an software implementation.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
Changes in V3:
* adding warning message to say that falling back to default sw pool
---
 lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 4846b89..8ab0eb1 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
 
 	rte_errno = rte_mempool_set_ops_byname(mp,
 			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
+
+	/* on error, try falling back to the software based default pool */
+	if (rte_errno == -EOPNOTSUPP) {
+		RTE_LOG(WARNING, MBUF, "Default HW Mempool not supported. "
+			"falling back to sw mempool \"ring_mp_mc\"");
+		rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);
+	}
+
 	if (rte_errno != 0) {
 		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
 		return NULL;
-- 
1.9.1

Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Olivier Matz]

Hi Hemant,

On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
> In the rte_pktmbuf_pool_create, if the default external mempool is
> not available, the implementation can default to "ring_mp_mc", which
> is an software implementation.
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> Changes in V3:
> * adding warning message to say that falling back to default sw pool
> ---
>  lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 4846b89..8ab0eb1 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
>  
>  	rte_errno = rte_mempool_set_ops_byname(mp,
>  			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
> +
> +	/* on error, try falling back to the software based default pool */
> +	if (rte_errno == -EOPNOTSUPP) {
> +		RTE_LOG(WARNING, MBUF, "Default HW Mempool not supported. "
> +			"falling back to sw mempool \"ring_mp_mc\"");
> +		rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);
> +	}
> +
>  	if (rte_errno != 0) {
>  		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>  		return NULL;
> 

Without adding a new method ".supported()", the first call to
rte_mempool_populate() could return the same error ENOTSUP. In this
case, it is still possible to fallback.

I've just submitted an RFC, which I think is quite linked:
http://dpdk.org/ml/archives/dev/2016-September/046974.html
Assuming a new parameter "mempool_ops" is added to
rte_pktmbuf_pool_create(), would it make sense to fallback to
"ring_mp_mc"? What about just returning ENOTSUP? The application could
do the job and decide which sw fallback to use.


Regards,
Olivier

Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

Hi Olivier

On 9/19/2016 7:27 PM, Olivier Matz wrote:
> Hi Hemant,
>
> On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
>> In the rte_pktmbuf_pool_create, if the default external mempool is
>> not available, the implementation can default to "ring_mp_mc", which
>> is an software implementation.
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>> Changes in V3:
>> * adding warning message to say that falling back to default sw pool
>> ---
>>  lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>> index 4846b89..8ab0eb1 100644
>> --- a/lib/librte_mbuf/rte_mbuf.c
>> +++ b/lib/librte_mbuf/rte_mbuf.c
>> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
>>
>>  	rte_errno = rte_mempool_set_ops_byname(mp,
>>  			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
>> +
>> +	/* on error, try falling back to the software based default pool */
>> +	if (rte_errno == -EOPNOTSUPP) {
>> +		RTE_LOG(WARNING, MBUF, "Default HW Mempool not supported. "
>> +			"falling back to sw mempool \"ring_mp_mc\"");
>> +		rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);
>> +	}
>> +
>>  	if (rte_errno != 0) {
>>  		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>>  		return NULL;
>>
>
> Without adding a new method ".supported()", the first call to
> rte_mempool_populate() could return the same error ENOTSUP. In this
> case, it is still possible to fallback.
>
It will be bit late.

On failure, than we have to set the default ops and do a goto before
rte_pktmbuf_pool_init(mp, &mbp_priv);


> I've just submitted an RFC, which I think is quite linked:
> http://dpdk.org/ml/archives/dev/2016-September/046974.html
> Assuming a new parameter "mempool_ops" is added to
> rte_pktmbuf_pool_create(), would it make sense to fallback to
> "ring_mp_mc"? What about just returning ENOTSUP? The application could
> do the job and decide which sw fallback to use.

We ran into this issue when trying to run the standard DPDK examples 
(l3fwd) in VM. Do you think, is it practical to add fallback handling in 
each of the DPDK examples?

Typically when someone is writing a application on host, he need not 
worry non-availability of the hw offloaded mempool. He may also want to 
run the same binary in virtual machine. In VM, it is not guaranteed that 
hw offloaded mempools will be available.

w.r.t your RFC, we can do this:
if the user has specified a mempool_ops in rte_pktmbuf_pool_create(), 
don't fallback. It will be responsibility for application to decide on 
calling again rte_pktmbuf_pool_create() with different mempool_ops.

>
>
> Regards,
> Olivier
>

Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

Hi Olivier,
	Any updates w.r.t this patch set?

Regards
Hemant
On 9/22/2016 6:42 PM, Hemant Agrawal wrote:
> Hi Olivier
>
> On 9/19/2016 7:27 PM, Olivier Matz wrote:
>> Hi Hemant,
>>
>> On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
>>> In the rte_pktmbuf_pool_create, if the default external mempool is
>>> not available, the implementation can default to "ring_mp_mc", which
>>> is an software implementation.
>>>
>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>> ---
>>> Changes in V3:
>>> * adding warning message to say that falling back to default sw pool
>>> ---
>>>  lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>>> index 4846b89..8ab0eb1 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.c
>>> +++ b/lib/librte_mbuf/rte_mbuf.c
>>> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name,
>>> unsigned n,
>>>
>>>      rte_errno = rte_mempool_set_ops_byname(mp,
>>>              RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
>>> +
>>> +    /* on error, try falling back to the software based default pool */
>>> +    if (rte_errno == -EOPNOTSUPP) {
>>> +        RTE_LOG(WARNING, MBUF, "Default HW Mempool not supported. "
>>> +            "falling back to sw mempool \"ring_mp_mc\"");
>>> +        rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);
>>> +    }
>>> +
>>>      if (rte_errno != 0) {
>>>          RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>>>          return NULL;
>>>
>>
>> Without adding a new method ".supported()", the first call to
>> rte_mempool_populate() could return the same error ENOTSUP. In this
>> case, it is still possible to fallback.
>>
> It will be bit late.
>
> On failure, than we have to set the default ops and do a goto before
> rte_pktmbuf_pool_init(mp, &mbp_priv);
>
>
>> I've just submitted an RFC, which I think is quite linked:
>> http://dpdk.org/ml/archives/dev/2016-September/046974.html
>> Assuming a new parameter "mempool_ops" is added to
>> rte_pktmbuf_pool_create(), would it make sense to fallback to
>> "ring_mp_mc"? What about just returning ENOTSUP? The application could
>> do the job and decide which sw fallback to use.
>
> We ran into this issue when trying to run the standard DPDK examples
> (l3fwd) in VM. Do you think, is it practical to add fallback handling in
> each of the DPDK examples?
>
> Typically when someone is writing a application on host, he need not
> worry non-availability of the hw offloaded mempool. He may also want to
> run the same binary in virtual machine. In VM, it is not guaranteed that
> hw offloaded mempools will be available.
>
> w.r.t your RFC, we can do this:
> if the user has specified a mempool_ops in rte_pktmbuf_pool_create(),
> don't fallback. It will be responsibility for application to decide on
> calling again rte_pktmbuf_pool_create() with different mempool_ops.
>
>>
>>
>> Regards,
>> Olivier
>>
>
>
>

Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Olivier Matz]

Hi Hemant,

Sorry for the late answer. Please see some comments inline.

On 10/13/2016 03:15 PM, Hemant Agrawal wrote:
> Hi Olivier,
>     Any updates w.r.t this patch set?
> 
> Regards
> Hemant
> On 9/22/2016 6:42 PM, Hemant Agrawal wrote:
>> Hi Olivier
>>
>> On 9/19/2016 7:27 PM, Olivier Matz wrote:
>>> Hi Hemant,
>>>
>>> On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
>>>> In the rte_pktmbuf_pool_create, if the default external mempool is
>>>> not available, the implementation can default to "ring_mp_mc", which
>>>> is an software implementation.
>>>>
>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>>> ---
>>>> Changes in V3:
>>>> * adding warning message to say that falling back to default sw pool
>>>> ---
>>>>  lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>>>> index 4846b89..8ab0eb1 100644
>>>> --- a/lib/librte_mbuf/rte_mbuf.c
>>>> +++ b/lib/librte_mbuf/rte_mbuf.c
>>>> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name,
>>>> unsigned n,
>>>>
>>>>      rte_errno = rte_mempool_set_ops_byname(mp,
>>>>              RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
>>>> +
>>>> +    /* on error, try falling back to the software based default
>>>> pool */
>>>> +    if (rte_errno == -EOPNOTSUPP) {
>>>> +        RTE_LOG(WARNING, MBUF, "Default HW Mempool not supported. "
>>>> +            "falling back to sw mempool \"ring_mp_mc\"");
>>>> +        rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc",
>>>> NULL);
>>>> +    }
>>>> +
>>>>      if (rte_errno != 0) {
>>>>          RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>>>>          return NULL;
>>>>
>>>
>>> Without adding a new method ".supported()", the first call to
>>> rte_mempool_populate() could return the same error ENOTSUP. In this
>>> case, it is still possible to fallback.
>>>
>> It will be bit late.
>>
>> On failure, than we have to set the default ops and do a goto before
>> rte_pktmbuf_pool_init(mp, &mbp_priv);

I still think we can do the job without adding the .supported() method.
The following code is just an (untested) example:

struct rte_mempool *
rte_pktmbuf_pool_create(const char *name, unsigned n,
    unsigned cache_size, uint16_t priv_size, uint16_t data_room_size,
    int socket_id)
{
    struct rte_mempool *mp;
    struct rte_pktmbuf_pool_private mbp_priv;
    unsigned elt_size;
    int ret;
    const char *ops[] = {
        RTE_MBUF_DEFAULT_MEMPOOL_OPS, "ring_mp_mc", NULL,
    };
    const char **op;

    if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
        RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
            priv_size);
        rte_errno = EINVAL;
        return NULL;
    }
    elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
        (unsigned)data_room_size;
    mbp_priv.mbuf_data_room_size = data_room_size;
    mbp_priv.mbuf_priv_size = priv_size;

    for (op = &ops[0]; *op != NULL; op++) {
        mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
            sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
        if (mp == NULL)
            return NULL;

        ret = rte_mempool_set_ops_byname(mp, *op, NULL);
        if (ret != 0) {
            RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
            rte_mempool_free(mp);
            if (ret == -ENOTSUP)
                continue;
            rte_errno = -ret;
            return NULL;
        }
        rte_pktmbuf_pool_init(mp, &mbp_priv);

        ret = rte_mempool_populate_default(mp);
        if (ret < 0) {
            rte_mempool_free(mp);
            if (ret == -ENOTSUP)
                continue;
            rte_errno = -ret;
            return NULL;
        }
    }

    rte_mempool_obj_iter(mp, rte_pktmbuf_init, NULL);

    return mp;
}


>>> I've just submitted an RFC, which I think is quite linked:
>>> http://dpdk.org/ml/archives/dev/2016-September/046974.html
>>> Assuming a new parameter "mempool_ops" is added to
>>> rte_pktmbuf_pool_create(), would it make sense to fallback to
>>> "ring_mp_mc"? What about just returning ENOTSUP? The application could
>>> do the job and decide which sw fallback to use.
>>
>> We ran into this issue when trying to run the standard DPDK examples
>> (l3fwd) in VM. Do you think, is it practical to add fallback handling in
>> each of the DPDK examples?

OK. What is still unclear for me, is how the software is aware of the
different hardware-assisted handlers. Moreover, we could imagine more
software handlers, which could be used depending on the use case.

I think this choice has to be made by the user or the application:

- the application may want to use a specific (sw or hw) handler: in
  this case, it want to be notified if it fails, instead of having
  a quiet fallback to ring_mp_mc
- if several handlers are available, the application may want to
  try them in a specific order
- maybe some handlers will have some limitations with some
  configurations or driver? The application could decide to use
  a different handler according the configuration.

So that's why I think this is an application decision.

>> Typically when someone is writing a application on host, he need not
>> worry non-availability of the hw offloaded mempool. He may also want to
>> run the same binary in virtual machine. In VM, it is not guaranteed that
>> hw offloaded mempools will be available.

Running the same binary is of course a need. But if your VM does not
provide the same virtualized hardware than the host, I think the command
line option makes sense.

AFAIU, on the host, you can use a hw mempool handler or a sw one, and
on the guest, only a sw one. So you need an option to select the
behavior you want on the host, without recompiling.

I understand that modifying all the applications is not a good option
either. I'm thinking a a EAL parameter that would allow to configure a
library (mbuf in this case). Something like kernel boot options.
Example: testpmd -l 2,4 --opt mbuf.handler="ring_mp_mc" -- [app args]

I don't know if this is feasible, this has to be discussed first, but
what do you think on the principle? The value could also be an
ordered list if we want a fallback, and this option could be overriden
by the application before creating the mbuf pool. There was some
discussions about a kind of dpdk global configuration some time ago.


Regards,
Olivier

Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

Hi Olivier,
	
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, October 14, 2016 5:41 PM
> > On 9/22/2016 6:42 PM, Hemant Agrawal wrote:
> >> Hi Olivier
> >>
> >> On 9/19/2016 7:27 PM, Olivier Matz wrote:
> >>> Hi Hemant,
> >>>
> >>> On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
> >>>> In the rte_pktmbuf_pool_create, if the default external mempool is
> >>>> not available, the implementation can default to "ring_mp_mc",
> >>>> which is an software implementation.
> >>>>
> >>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >>>> ---
> >>>> Changes in V3:
> >>>> * adding warning message to say that falling back to default sw
> >>>> pool
> >>>> ---
> >>>>  lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
> >>>>  1 file changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/lib/librte_mbuf/rte_mbuf.c
> >>>> b/lib/librte_mbuf/rte_mbuf.c index 4846b89..8ab0eb1 100644
> >>>> --- a/lib/librte_mbuf/rte_mbuf.c
> >>>> +++ b/lib/librte_mbuf/rte_mbuf.c
> >>>> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name,
> >>>> unsigned n,
> >>>>
> >>>>      rte_errno = rte_mempool_set_ops_byname(mp,
> >>>>              RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
> >>>> +
> >>>> +    /* on error, try falling back to the software based default
> >>>> pool */
> >>>> +    if (rte_errno == -EOPNOTSUPP) {
> >>>> +        RTE_LOG(WARNING, MBUF, "Default HW Mempool not supported. "
> >>>> +            "falling back to sw mempool \"ring_mp_mc\"");
> >>>> +        rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc",
> >>>> NULL);
> >>>> +    }
> >>>> +
> >>>>      if (rte_errno != 0) {
> >>>>          RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> >>>>          return NULL;
> >>>>
> >>>
> >>> Without adding a new method ".supported()", the first call to
> >>> rte_mempool_populate() could return the same error ENOTSUP. In this
> >>> case, it is still possible to fallback.
> >>>
> >> It will be bit late.
> >>
> >> On failure, than we have to set the default ops and do a goto before
> >> rte_pktmbuf_pool_init(mp, &mbp_priv);
> 
> I still think we can do the job without adding the .supported() method.
> The following code is just an (untested) example:
> 
> struct rte_mempool *
> rte_pktmbuf_pool_create(const char *name, unsigned n,
>     unsigned cache_size, uint16_t priv_size, uint16_t data_room_size,
>     int socket_id)
> {
>     struct rte_mempool *mp;
>     struct rte_pktmbuf_pool_private mbp_priv;
>     unsigned elt_size;
>     int ret;
>     const char *ops[] = {
>         RTE_MBUF_DEFAULT_MEMPOOL_OPS, "ring_mp_mc", NULL,
>     };
>     const char **op;
> 
>     if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
>         RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
>             priv_size);
>         rte_errno = EINVAL;
>         return NULL;
>     }
>     elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
>         (unsigned)data_room_size;
>     mbp_priv.mbuf_data_room_size = data_room_size;
>     mbp_priv.mbuf_priv_size = priv_size;
> 
>     for (op = &ops[0]; *op != NULL; op++) {
>         mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
>             sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
>         if (mp == NULL)
>             return NULL;
> 
>         ret = rte_mempool_set_ops_byname(mp, *op, NULL);
>         if (ret != 0) {
>             RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>             rte_mempool_free(mp);
>             if (ret == -ENOTSUP)
>                 continue;
>             rte_errno = -ret;
>             return NULL;
>         }
>         rte_pktmbuf_pool_init(mp, &mbp_priv);
> 
>         ret = rte_mempool_populate_default(mp);
>         if (ret < 0) {
>             rte_mempool_free(mp);
>             if (ret == -ENOTSUP)
>                 continue;
>             rte_errno = -ret;
>             return NULL;
>         }
>     }
> 
>     rte_mempool_obj_iter(mp, rte_pktmbuf_init, NULL);
> 
>     return mp;
> }
> 
> 
[Hemant]  This look fine to me. Please submit a patch for the same. 

> >>> I've just submitted an RFC, which I think is quite linked:
> >>> http://dpdk.org/ml/archives/dev/2016-September/046974.html
> >>> Assuming a new parameter "mempool_ops" is added to
> >>> rte_pktmbuf_pool_create(), would it make sense to fallback to
> >>> "ring_mp_mc"? What about just returning ENOTSUP? The application
> >>> could do the job and decide which sw fallback to use.
> >>
> >> We ran into this issue when trying to run the standard DPDK examples
> >> (l3fwd) in VM. Do you think, is it practical to add fallback handling
> >> in each of the DPDK examples?
> 
> OK. What is still unclear for me, is how the software is aware of the different
> hardware-assisted handlers. Moreover, we could imagine more software
> handlers, which could be used depending on the use case.
> 
> I think this choice has to be made by the user or the application:
> 
> - the application may want to use a specific (sw or hw) handler: in
>   this case, it want to be notified if it fails, instead of having
>   a quiet fallback to ring_mp_mc
> - if several handlers are available, the application may want to
>   try them in a specific order
> - maybe some handlers will have some limitations with some
>   configurations or driver? The application could decide to use
>   a different handler according the configuration.
> 
> So that's why I think this is an application decision.
> 
[Hemant]  We should simplify it:  if the application has supplied the handler, it is application's responsibility to take care of failure. Only if the application want to use the default handler, the implementation can fallback.  The fallback handler can again be configurable. 

> >> Typically when someone is writing a application on host, he need not
> >> worry non-availability of the hw offloaded mempool. He may also want
> >> to run the same binary in virtual machine. In VM, it is not
> >> guaranteed that hw offloaded mempools will be available.
> 
> Running the same binary is of course a need. But if your VM does not provide
> the same virtualized hardware than the host, I think the command line option
> makes sense.
> 
> AFAIU, on the host, you can use a hw mempool handler or a sw one, and on the
> guest, only a sw one. So you need an option to select the behavior you want on
> the host, without recompiling.
> 
> I understand that modifying all the applications is not a good option either. I'm
> thinking a a EAL parameter that would allow to configure a library (mbuf in this
> case). Something like kernel boot options.
> Example: testpmd -l 2,4 --opt mbuf.handler="ring_mp_mc" -- [app args]
> 
> I don't know if this is feasible, this has to be discussed first, but what do you
> think on the principle? The value could also be an ordered list if we want a
> fallback, and this option could be overriden by the application before creating
> the mbuf pool. There was some discussions about a kind of dpdk global
> configuration some time ago.
> 
[Hemant] I agree that command line option provide a better control in this case. 
On the flipside, We need to be careful that we do not end up having too many command line options. 

> 
> Regards,
> Olivier

Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Olivier Matz]

Hi Hemant,

Back on this topic, please see some comments below.

On 11/07/2016 01:30 PM, Hemant Agrawal wrote:
> Hi Olivier,
> 	
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Friday, October 14, 2016 5:41 PM
>>> On 9/22/2016 6:42 PM, Hemant Agrawal wrote:
>>>> Hi Olivier
>>>>
>>>> On 9/19/2016 7:27 PM, Olivier Matz wrote:
>>>>> Hi Hemant,
>>>>>
>>>>> On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
>>>>>> In the rte_pktmbuf_pool_create, if the default external mempool is
>>>>>> not available, the implementation can default to "ring_mp_mc",
>>>>>> which is an software implementation.
>>>>>>
>>>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>>>>> ---
>>>>>> Changes in V3:
>>>>>> * adding warning message to say that falling back to default sw
>>>>>> pool
>>>>>> ---
>>>>>>  lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
>>>>>>  1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.c
>>>>>> b/lib/librte_mbuf/rte_mbuf.c index 4846b89..8ab0eb1 100644
>>>>>> --- a/lib/librte_mbuf/rte_mbuf.c
>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.c
>>>>>> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name,
>>>>>> unsigned n,
>>>>>>
>>>>>>      rte_errno = rte_mempool_set_ops_byname(mp,
>>>>>>              RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
>>>>>> +
>>>>>> +    /* on error, try falling back to the software based default
>>>>>> pool */
>>>>>> +    if (rte_errno == -EOPNOTSUPP) {
>>>>>> +        RTE_LOG(WARNING, MBUF, "Default HW Mempool not supported. "
>>>>>> +            "falling back to sw mempool \"ring_mp_mc\"");
>>>>>> +        rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc",
>>>>>> NULL);
>>>>>> +    }
>>>>>> +
>>>>>>      if (rte_errno != 0) {
>>>>>>          RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>>>>>>          return NULL;
>>>>>>
>>>>>
>>>>> Without adding a new method ".supported()", the first call to
>>>>> rte_mempool_populate() could return the same error ENOTSUP. In this
>>>>> case, it is still possible to fallback.
>>>>>
>>>> It will be bit late.
>>>>
>>>> On failure, than we have to set the default ops and do a goto before
>>>> rte_pktmbuf_pool_init(mp, &mbp_priv);
>>
>> I still think we can do the job without adding the .supported() method.
>> The following code is just an (untested) example:
>>
>> struct rte_mempool *
>> rte_pktmbuf_pool_create(const char *name, unsigned n,
>>     unsigned cache_size, uint16_t priv_size, uint16_t data_room_size,
>>     int socket_id)
>> {
>>     struct rte_mempool *mp;
>>     struct rte_pktmbuf_pool_private mbp_priv;
>>     unsigned elt_size;
>>     int ret;
>>     const char *ops[] = {
>>         RTE_MBUF_DEFAULT_MEMPOOL_OPS, "ring_mp_mc", NULL,
>>     };
>>     const char **op;
>>
>>     if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
>>         RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
>>             priv_size);
>>         rte_errno = EINVAL;
>>         return NULL;
>>     }
>>     elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
>>         (unsigned)data_room_size;
>>     mbp_priv.mbuf_data_room_size = data_room_size;
>>     mbp_priv.mbuf_priv_size = priv_size;
>>
>>     for (op = &ops[0]; *op != NULL; op++) {
>>         mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
>>             sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
>>         if (mp == NULL)
>>             return NULL;
>>
>>         ret = rte_mempool_set_ops_byname(mp, *op, NULL);
>>         if (ret != 0) {
>>             RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
>>             rte_mempool_free(mp);
>>             if (ret == -ENOTSUP)
>>                 continue;
>>             rte_errno = -ret;
>>             return NULL;
>>         }
>>         rte_pktmbuf_pool_init(mp, &mbp_priv);
>>
>>         ret = rte_mempool_populate_default(mp);
>>         if (ret < 0) {
>>             rte_mempool_free(mp);
>>             if (ret == -ENOTSUP)
>>                 continue;
>>             rte_errno = -ret;
>>             return NULL;
>>         }
>>     }
>>
>>     rte_mempool_obj_iter(mp, rte_pktmbuf_init, NULL);
>>
>>     return mp;
>> }
>>
>>
> [Hemant]  This look fine to me. Please submit a patch for the same. 
> 
>>>>> I've just submitted an RFC, which I think is quite linked:
>>>>> http://dpdk.org/ml/archives/dev/2016-September/046974.html
>>>>> Assuming a new parameter "mempool_ops" is added to
>>>>> rte_pktmbuf_pool_create(), would it make sense to fallback to
>>>>> "ring_mp_mc"? What about just returning ENOTSUP? The application
>>>>> could do the job and decide which sw fallback to use.
>>>>
>>>> We ran into this issue when trying to run the standard DPDK examples
>>>> (l3fwd) in VM. Do you think, is it practical to add fallback handling
>>>> in each of the DPDK examples?
>>
>> OK. What is still unclear for me, is how the software is aware of the different
>> hardware-assisted handlers. Moreover, we could imagine more software
>> handlers, which could be used depending on the use case.
>>
>> I think this choice has to be made by the user or the application:
>>
>> - the application may want to use a specific (sw or hw) handler: in
>>   this case, it want to be notified if it fails, instead of having
>>   a quiet fallback to ring_mp_mc
>> - if several handlers are available, the application may want to
>>   try them in a specific order
>> - maybe some handlers will have some limitations with some
>>   configurations or driver? The application could decide to use
>>   a different handler according the configuration.
>>
>> So that's why I think this is an application decision.
>>
> [Hemant]  We should simplify it:  if the application has supplied the handler, it is application's responsibility to take care of failure. Only if the application want to use the default handler, the implementation can fallback.  The fallback handler can again be configurable. 

Honestly, I'm not very convinced that having a quiet fallback is
the proper solution: the application won't be notified that the
fallback occured.

I understand your patch solves your use-case, but my fear is that
we just integrate this minimal patch and never do the harder work
of solving all the use-cases. I think we need to give the tools to
the applications to control what occurs because we also need to solve
these issues:
- you have a hardware handler, but you want to use the software
  handler
- you have several software handlers, and you (as an administrator) or
  the application knows which one is the best to use in your case.

An alternative (which I don't like either) to solve your issue without
modifying the mbuf code is to have your own handler fallbacking to
ring_mp_mc.


>>>> Typically when someone is writing a application on host, he need not
>>>> worry non-availability of the hw offloaded mempool. He may also want
>>>> to run the same binary in virtual machine. In VM, it is not
>>>> guaranteed that hw offloaded mempools will be available.
>>
>> Running the same binary is of course a need. But if your VM does not provide
>> the same virtualized hardware than the host, I think the command line option
>> makes sense.
>>
>> AFAIU, on the host, you can use a hw mempool handler or a sw one, and on the
>> guest, only a sw one. So you need an option to select the behavior you want on
>> the host, without recompiling.
>>
>> I understand that modifying all the applications is not a good option either. I'm
>> thinking a a EAL parameter that would allow to configure a library (mbuf in this
>> case). Something like kernel boot options.
>> Example: testpmd -l 2,4 --opt mbuf.handler="ring_mp_mc" -- [app args]
>>
>> I don't know if this is feasible, this has to be discussed first, but what do you
>> think on the principle? The value could also be an ordered list if we want a
>> fallback, and this option could be overriden by the application before creating
>> the mbuf pool. There was some discussions about a kind of dpdk global
>> configuration some time ago.
>>
> [Hemant] I agree that command line option provide a better control in this case. 
> On the flipside, We need to be careful that we do not end up having too many command line options. 

Yes, we should avoid having too many command line arguments.
I think we should go in the direction of having a sort of key/value
database in EAL. It could be set by:
- a generic command line argument
- a config file (maybe later)
- the application through an API

Then, it would be used by:
- dpdk libraries
- the application (maybe)

This has been discussed some times, the latest was probably:
http://dpdk.org/ml/archives/dev/2016-June/040079.html

I think it would be a good tool for dpdk configurability, and it would
help to remove some compile-time options and maybe some eal options.
Unfortunately, I don't have the time to work on this at the moment.

Regards,
Olivier

Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error

[Hemant Agrawal]

Hi Olivier,
	Apology for a delayed response.

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, November 22, 2016 2:55 PM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; david.hunt@intel.com
> Subject: Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for
> mempool ops error
> 
> Hi Hemant,
> 
> Back on this topic, please see some comments below.
> 
> On 11/07/2016 01:30 PM, Hemant Agrawal wrote:
> > Hi Olivier,
> >
> >> -----Original Message-----
> >> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >> Sent: Friday, October 14, 2016 5:41 PM
> >>> On 9/22/2016 6:42 PM, Hemant Agrawal wrote:
> >>>> Hi Olivier
> >>>>
> >>>> On 9/19/2016 7:27 PM, Olivier Matz wrote:
> >>>>> Hi Hemant,
> >>>>>
> >>>>> On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
> >>>>>> In the rte_pktmbuf_pool_create, if the default external mempool
> >>>>>> is not available, the implementation can default to "ring_mp_mc",
> >>>>>> which is an software implementation.
> >>>>>>
> >>>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >>>>>> ---
> >>>>>> Changes in V3:
> >>>>>> * adding warning message to say that falling back to default sw
> >>>>>> pool
> >>>>>> ---
> >>>>>>  lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
> >>>>>>  1 file changed, 8 insertions(+)
> >>>>>>
> >>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.c
> >>>>>> b/lib/librte_mbuf/rte_mbuf.c index 4846b89..8ab0eb1 100644
> >>>>>> --- a/lib/librte_mbuf/rte_mbuf.c
> >>>>>> +++ b/lib/librte_mbuf/rte_mbuf.c
> >>>>>> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name,
> >>>>>> unsigned n,
> >>>>>>
> >>>>>>      rte_errno = rte_mempool_set_ops_byname(mp,
> >>>>>>              RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
> >>>>>> +
> >>>>>> +    /* on error, try falling back to the software based default
> >>>>>> pool */
> >>>>>> +    if (rte_errno == -EOPNOTSUPP) {
> >>>>>> +        RTE_LOG(WARNING, MBUF, "Default HW Mempool not
> supported. "
> >>>>>> +            "falling back to sw mempool \"ring_mp_mc\"");
> >>>>>> +        rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc",
> >>>>>> NULL);
> >>>>>> +    }
> >>>>>> +
> >>>>>>      if (rte_errno != 0) {
> >>>>>>          RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> >>>>>>          return NULL;
> >>>>>>
> >>>>>
> >>>>> Without adding a new method ".supported()", the first call to
> >>>>> rte_mempool_populate() could return the same error ENOTSUP. In
> >>>>> this case, it is still possible to fallback.
> >>>>>
> >>>> It will be bit late.
> >>>>
> >>>> On failure, than we have to set the default ops and do a goto
> >>>> before rte_pktmbuf_pool_init(mp, &mbp_priv);
> >>
> >> I still think we can do the job without adding the .supported() method.
> >> The following code is just an (untested) example:
> >>
> >> struct rte_mempool *
> >> rte_pktmbuf_pool_create(const char *name, unsigned n,
> >>     unsigned cache_size, uint16_t priv_size, uint16_t data_room_size,
> >>     int socket_id)
> >> {
> >>     struct rte_mempool *mp;
> >>     struct rte_pktmbuf_pool_private mbp_priv;
> >>     unsigned elt_size;
> >>     int ret;
> >>     const char *ops[] = {
> >>         RTE_MBUF_DEFAULT_MEMPOOL_OPS, "ring_mp_mc", NULL,
> >>     };
> >>     const char **op;
> >>
> >>     if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
> >>         RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
> >>             priv_size);
> >>         rte_errno = EINVAL;
> >>         return NULL;
> >>     }
> >>     elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
> >>         (unsigned)data_room_size;
> >>     mbp_priv.mbuf_data_room_size = data_room_size;
> >>     mbp_priv.mbuf_priv_size = priv_size;
> >>
> >>     for (op = &ops[0]; *op != NULL; op++) {
> >>         mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
> >>             sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
> >>         if (mp == NULL)
> >>             return NULL;
> >>
> >>         ret = rte_mempool_set_ops_byname(mp, *op, NULL);
> >>         if (ret != 0) {
> >>             RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> >>             rte_mempool_free(mp);
> >>             if (ret == -ENOTSUP)
> >>                 continue;
> >>             rte_errno = -ret;
> >>             return NULL;
> >>         }
> >>         rte_pktmbuf_pool_init(mp, &mbp_priv);
> >>
> >>         ret = rte_mempool_populate_default(mp);
> >>         if (ret < 0) {
> >>             rte_mempool_free(mp);
> >>             if (ret == -ENOTSUP)
> >>                 continue;
> >>             rte_errno = -ret;
> >>             return NULL;
> >>         }
> >>     }
> >>
> >>     rte_mempool_obj_iter(mp, rte_pktmbuf_init, NULL);
> >>
> >>     return mp;
> >> }
> >>
> >>
> > [Hemant]  This look fine to me. Please submit a patch for the same.
> >
> >>>>> I've just submitted an RFC, which I think is quite linked:
> >>>>> http://dpdk.org/ml/archives/dev/2016-September/046974.html
> >>>>> Assuming a new parameter "mempool_ops" is added to
> >>>>> rte_pktmbuf_pool_create(), would it make sense to fallback to
> >>>>> "ring_mp_mc"? What about just returning ENOTSUP? The application
> >>>>> could do the job and decide which sw fallback to use.
> >>>>
> >>>> We ran into this issue when trying to run the standard DPDK
> >>>> examples
> >>>> (l3fwd) in VM. Do you think, is it practical to add fallback
> >>>> handling in each of the DPDK examples?
> >>
> >> OK. What is still unclear for me, is how the software is aware of the
> >> different hardware-assisted handlers. Moreover, we could imagine more
> >> software handlers, which could be used depending on the use case.
> >>
> >> I think this choice has to be made by the user or the application:
> >>
> >> - the application may want to use a specific (sw or hw) handler: in
> >>   this case, it want to be notified if it fails, instead of having
> >>   a quiet fallback to ring_mp_mc
> >> - if several handlers are available, the application may want to
> >>   try them in a specific order
> >> - maybe some handlers will have some limitations with some
> >>   configurations or driver? The application could decide to use
> >>   a different handler according the configuration.
> >>
> >> So that's why I think this is an application decision.
> >>
> > [Hemant]  We should simplify it:  if the application has supplied the handler, it
> is application's responsibility to take care of failure. Only if the application want
> to use the default handler, the implementation can fallback.  The fallback
> handler can again be configurable.
> 
> Honestly, I'm not very convinced that having a quiet fallback is the proper
> solution: the application won't be notified that the fallback occured.
> 
> I understand your patch solves your use-case, but my fear is that we just
> integrate this minimal patch and never do the harder work of solving all the use-
> cases. I think we need to give the tools to the applications to control what
> occurs because we also need to solve these issues:
> - you have a hardware handler, but you want to use the software
>   handler
> - you have several software handlers, and you (as an administrator) or
>   the application knows which one is the best to use in your case.
> 
> An alternative (which I don't like either) to solve your issue without modifying
> the mbuf code is to have your own handler fallbacking to ring_mp_mc.
> 
[Hemant] are you suggesting that we also configure a fallback handler in addition to default? 

I agree that we need to provide full flexibility to the applications, who are willing to implement the fallback mechanism. 

> 
> >>>> Typically when someone is writing a application on host, he need
> >>>> not worry non-availability of the hw offloaded mempool. He may also
> >>>> want to run the same binary in virtual machine. In VM, it is not
> >>>> guaranteed that hw offloaded mempools will be available.
> >>
> >> Running the same binary is of course a need. But if your VM does not
> >> provide the same virtualized hardware than the host, I think the
> >> command line option makes sense.
> >>
> >> AFAIU, on the host, you can use a hw mempool handler or a sw one, and
> >> on the guest, only a sw one. So you need an option to select the
> >> behavior you want on the host, without recompiling.
> >>
> >> I understand that modifying all the applications is not a good option
> >> either. I'm thinking a a EAL parameter that would allow to configure
> >> a library (mbuf in this case). Something like kernel boot options.
> >> Example: testpmd -l 2,4 --opt mbuf.handler="ring_mp_mc" -- [app args]
> >>
> >> I don't know if this is feasible, this has to be discussed first, but
> >> what do you think on the principle? The value could also be an
> >> ordered list if we want a fallback, and this option could be
> >> overriden by the application before creating the mbuf pool. There was
> >> some discussions about a kind of dpdk global configuration some time ago.
> >>
> > [Hemant] I agree that command line option provide a better control in this
> case.
> > On the flipside, We need to be careful that we do not end up having too many
> command line options.
> 
> Yes, we should avoid having too many command line arguments.
> I think we should go in the direction of having a sort of key/value database in
> EAL. It could be set by:
> - a generic command line argument
> - a config file (maybe later)
> - the application through an API
> 
> Then, it would be used by:
> - dpdk libraries
> - the application (maybe)
> 
> This has been discussed some times, the latest was probably:
> http://dpdk.org/ml/archives/dev/2016-June/040079.html
> 
> I think it would be a good tool for dpdk configurability, and it would help to
> remove some compile-time options and maybe some eal options.
> Unfortunately, I don't have the time to work on this at the moment.
> 
> Regards,
> Olivier