[dpdk-dev] [PATCH 1/2] mempool/dpaa2: fix the return value for alloc fail

[dpdk-dev] [PATCH 1/2] mempool/dpaa2: fix the return value for alloc fail

[Hemant Agrawal]

In case the alloc api is not able to allocate the required
number of buffer, it can return '0', which will not indicate
the failure to the calling function.
This patch fix the return value to indicate the failure.

Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
CC: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
index 5a5d6aa..60dd1c0 100644
--- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
+++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
@@ -294,7 +294,7 @@
 			/* Releasing all buffers allocated */
 			rte_dpaa2_mbuf_release(pool, obj_table, bpid,
 					   bp_info->meta_data_size, n);
-			return ret;
+			return -1;
 		}
 		/* assigning mbuf from the acquired objects */
 		for (i = 0; (i < ret) && bufs[i]; i++) {
-- 
1.9.1

[dpdk-dev] [PATCH 2/2] mempool/dpaa2: improving the alloc/free logging

[Hemant Agrawal]

Debug logs are helpful for better debugging. Alloc
was having the logs, but logs were not present in free routines.

This patch add support for debug mode logs in free routine.
Also, changing the log category to DRV instead of TX.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
index 60dd1c0..e00ed5d 100644
--- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
+++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
@@ -309,8 +309,8 @@
 
 #ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
 	alloc += n;
-	PMD_TX_LOG(DEBUG, "Total = %d , req = %d done = %d",
-		   alloc, count, n);
+	PMD_DRV_LOG(DEBUG, "Total = %d , req = %d done = %d",
+		    alloc, count, n);
 #endif
 	return 0;
 }
@@ -320,6 +320,9 @@
 		  void * const *obj_table, unsigned int n)
 {
 	struct dpaa2_bp_info *bp_info;
+#ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
+	static int freed;
+#endif
 
 	bp_info = mempool_to_bpinfo(pool);
 	if (!(bp_info->bp_list)) {
@@ -329,6 +332,11 @@
 	rte_dpaa2_mbuf_release(pool, obj_table, bp_info->bpid,
 			   bp_info->meta_data_size, n);
 
+#ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
+	freed += n;
+	PMD_DRV_LOG(DEBUG, "Total = %d , done = %d",
+		    freed, n);
+#endif
 	return 0;
 }
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH 2/2] mempool/dpaa2: improving the alloc/free logging

[Olivier Matz]

Hi Hemant,

On Tue, 23 May 2017 15:03:48 +0530, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> Debug logs are helpful for better debugging. Alloc
> was having the logs, but logs were not present in free routines.
> 
> This patch add support for debug mode logs in free routine.
> Also, changing the log category to DRV instead of TX.
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
> index 60dd1c0..e00ed5d 100644
> --- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
> +++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
> @@ -309,8 +309,8 @@
>  
>  #ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
>  	alloc += n;
> -	PMD_TX_LOG(DEBUG, "Total = %d , req = %d done = %d",
> -		   alloc, count, n);
> +	PMD_DRV_LOG(DEBUG, "Total = %d , req = %d done = %d",
> +		    alloc, count, n);
>  #endif
>  	return 0;
>  }

Since we are in a mempool driver, we may not want to use PMD_*_LOG()
functions.

Also, I don't see where these macros are defined for this file, but maybe
I miss something that is right under my nose ;)

Anyway, I suggest to use your own macros for dpaa mempool instead,
and if possible:
  - avoid compilation options as much as possible
  - RTE_LOG() for logs that cannot occur in dataplane (they can be enabled
    dynamically)
  - RTE_LOG_DP() for dataplane logs: these are stripped at compilation
    time, unless your log level <= RTE_LOG_DP_LEVEL


> @@ -320,6 +320,9 @@
>  		  void * const *obj_table, unsigned int n)
>  {
>  	struct dpaa2_bp_info *bp_info;
> +#ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
> +	static int freed;
> +#endif
>  
>  	bp_info = mempool_to_bpinfo(pool);
>  	if (!(bp_info->bp_list)) {
> @@ -329,6 +332,11 @@
>  	rte_dpaa2_mbuf_release(pool, obj_table, bp_info->bpid,
>  			   bp_info->meta_data_size, n);
>  
> +#ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
> +	freed += n;
> +	PMD_DRV_LOG(DEBUG, "Total = %d , done = %d",
> +		    freed, n);
> +#endif
>  	return 0;
>  }
>  

Re: [dpdk-dev] [PATCH 2/2] mempool/dpaa2: improving the alloc/free logging

[Hemant Agrawal]

On 6/8/2017 3:38 PM, Olivier Matz wrote:
> Hi Hemant,
>
> On Tue, 23 May 2017 15:03:48 +0530, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>> Debug logs are helpful for better debugging. Alloc
>> was having the logs, but logs were not present in free routines.
>>
>> This patch add support for debug mode logs in free routine.
>> Also, changing the log category to DRV instead of TX.
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>>  drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
>> index 60dd1c0..e00ed5d 100644
>> --- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
>> +++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
>> @@ -309,8 +309,8 @@
>>
>>  #ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
>>  	alloc += n;
>> -	PMD_TX_LOG(DEBUG, "Total = %d , req = %d done = %d",
>> -		   alloc, count, n);
>> +	PMD_DRV_LOG(DEBUG, "Total = %d , req = %d done = %d",
>> +		    alloc, count, n);
>>  #endif
>>  	return 0;
>>  }
>
> Since we are in a mempool driver, we may not want to use PMD_*_LOG()
> functions.
>
> Also, I don't see where these macros are defined for this file, but maybe
> I miss something that is right under my nose ;)
>
These are being currently being driven from the bus logs.

> Anyway, I suggest to use your own macros for dpaa mempool instead,
> and if possible:
>   - avoid compilation options as much as possible
>   - RTE_LOG() for logs that cannot occur in dataplane (they can be enabled
>     dynamically)
>   - RTE_LOG_DP() for dataplane logs: these are stripped at compilation
>     time, unless your log level <= RTE_LOG_DP_LEVEL
>
your suggestion is good. I will rework the whole file and submit it later.

>
>> @@ -320,6 +320,9 @@
>>  		  void * const *obj_table, unsigned int n)
>>  {
>>  	struct dpaa2_bp_info *bp_info;
>> +#ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
>> +	static int freed;
>> +#endif
>>
>>  	bp_info = mempool_to_bpinfo(pool);
>>  	if (!(bp_info->bp_list)) {
>> @@ -329,6 +332,11 @@
>>  	rte_dpaa2_mbuf_release(pool, obj_table, bp_info->bpid,
>>  			   bp_info->meta_data_size, n);
>>
>> +#ifdef RTE_LIBRTE_DPAA2_DEBUG_DRIVER
>> +	freed += n;
>> +	PMD_DRV_LOG(DEBUG, "Total = %d , done = %d",
>> +		    freed, n);
>> +#endif
>>  	return 0;
>>  }
>>
>
>

Re: [dpdk-dev] [PATCH 1/2] mempool/dpaa2: fix the return value for alloc fail

[Olivier Matz]

Hi Hemant,

On Tue, 23 May 2017 15:03:47 +0530, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> In case the alloc api is not able to allocate the required
> number of buffer, it can return '0', which will not indicate
> the failure to the calling function.
> This patch fix the return value to indicate the failure.
> 
> Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
> CC: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
> index 5a5d6aa..60dd1c0 100644
> --- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
> +++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
> @@ -294,7 +294,7 @@
>  			/* Releasing all buffers allocated */
>  			rte_dpaa2_mbuf_release(pool, obj_table, bpid,
>  					   bp_info->meta_data_size, n);
> -			return ret;
> +			return -1;
>  		}
>  		/* assigning mbuf from the acquired objects */
>  		for (i = 0; (i < ret) && bufs[i]; i++) {

Wouldn't it be better to return an errno as done in ring handler
(see common_ring_mp_enqueue()) or in stack handler (see stack_alloc())?

Olivier

[dpdk-dev] [PATCH v2 1/2] mempool/dpaa2: fix the return value for alloc fail

[Hemant Agrawal]

In case the alloc api is not able to allocate the required
number of buffer, it can return '0', which will not indicate
the failure to the calling function.
This patch fix the return value to indicate the failure.

Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
CC: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
index 5a5d6aa..27ed5a9 100644
--- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
+++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
@@ -294,7 +294,7 @@ rte_dpaa2_mbuf_alloc_bulk(struct rte_mempool *pool,
 			/* Releasing all buffers allocated */
 			rte_dpaa2_mbuf_release(pool, obj_table, bpid,
 					   bp_info->meta_data_size, n);
-			return ret;
+			return -ENOBUFS;
 		}
 		/* assigning mbuf from the acquired objects */
 		for (i = 0; (i < ret) && bufs[i]; i++) {
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/2] mempool/dpaa2: fix the incorrect free usages for bplist

[Hemant Agrawal]

The dpaa2_bp_list is being allocated using "rte_malloc",
but the free is done using "free". Fixing it to use
"rte_free".

Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
CC: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
index 27ed5a9..62a2d25 100644
--- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
+++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
@@ -161,7 +161,7 @@ rte_hw_mbuf_free_pool(struct rte_mempool *mp)
 		while (temp) {
 			if (temp == bp) {
 				prev->next = temp->next;
-				free(bp);
+				rte_free(bp);
 				break;
 			}
 			prev = temp;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 2/2] mempool/dpaa2: fix the incorrect free usages for bplist

[Olivier Matz]

On Thu, 22 Jun 2017 18:18:08 +0530, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> The dpaa2_bp_list is being allocated using "rte_malloc",
> but the free is done using "free". Fixing it to use
> "rte_free".
> 
> Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
> CC: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v2 2/2] mempool/dpaa2: fix the incorrect free usages for bplist

[Shreyansh Jain]

On Thursday 22 June 2017 06:18 PM, Hemant Agrawal wrote:
> The dpaa2_bp_list is being allocated using "rte_malloc",
> but the free is done using "free". Fixing it to use
> "rte_free".
> 
> Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
> CC: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---

Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>

Re: [dpdk-dev] [PATCH v2 1/2] mempool/dpaa2: fix the return value for alloc fail

[Olivier Matz]

On Thu, 22 Jun 2017 18:18:07 +0530, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> In case the alloc api is not able to allocate the required
> number of buffer, it can return '0', which will not indicate
> the failure to the calling function.
> This patch fix the return value to indicate the failure.
> 
> Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
> CC: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v2 1/2] mempool/dpaa2: fix the return value for alloc fail

[Shreyansh Jain]

On Thursday 22 June 2017 06:18 PM, Hemant Agrawal wrote:
> In case the alloc api is not able to allocate the required
> number of buffer, it can return '0', which will not indicate
> the failure to the calling function.
> This patch fix the return value to indicate the failure.
> 
> Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
> CC: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---

Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH v2 1/2] mempool/dpaa2: fix the return value for alloc fail

[Thomas Monjalon]

18/07/2017 17:47, Shreyansh Jain:
> On Thursday 22 June 2017 06:18 PM, Hemant Agrawal wrote:
> > In case the alloc api is not able to allocate the required
> > number of buffer, it can return '0', which will not indicate
> > the failure to the calling function.
> > This patch fix the return value to indicate the failure.
> > 
> > Fixes: 5dc43d22b5ad ("mempool/dpaa2: add hardware offloaded mempool")
> > CC: stable@dpdk.org
> > 
> > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> 
> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>

Series applied, thanks