[dpdk-dev] [PATCH] enic: don't set enic->config.rq_desc_count in enic_alloc_rq()

[dpdk-dev] [PATCH] enic: don't set enic->config.rq_desc_count in enic_alloc_rq()

[John Daley]

From: Nelson Escobar <neescoba@cisco.com>

When the requested number of rx descriptors was less than the amount
configured on the vic, enic_alloc_rq() was incorrectly setting
enic->config.rq_desc_count to the lower value.  This screwed up later
calls to enic_alloc_rq().

Signed-off-by: Nelson Escobar <neescoba@cisco.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic_main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 9fff020..cd7857f 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -524,24 +524,22 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 				"policy.  Applying the value in the adapter "\
 				"policy (%d).\n",
 				queue_idx, nb_desc, enic->config.rq_desc_count);
-		} else if (nb_desc != enic->config.rq_desc_count) {
-			enic->config.rq_desc_count = nb_desc;
-			dev_info(enic,
-				"RX Queues - effective number of descs:%d\n",
-				nb_desc);
+			nb_desc = enic->config.rq_desc_count;
 		}
+		dev_info(enic, "RX Queues - effective number of descs:%d\n",
+			 nb_desc);
 	}
 
 	/* Allocate queue resources */
 	rc = vnic_rq_alloc(enic->vdev, rq, queue_idx,
-		enic->config.rq_desc_count, sizeof(struct rq_enet_desc));
+		nb_desc, sizeof(struct rq_enet_desc));
 	if (rc) {
 		dev_err(enic, "error in allocation of rq\n");
 		goto err_exit;
 	}
 
 	rc = vnic_cq_alloc(enic->vdev, &enic->cq[queue_idx], queue_idx,
-		socket_id, enic->config.rq_desc_count,
+		socket_id, nb_desc,
 		sizeof(struct cq_enet_rq_desc));
 	if (rc) {
 		dev_err(enic, "error in allocation of cq for rq\n");
@@ -550,7 +548,7 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 
 	/* Allocate the mbuf ring */
 	rq->mbuf_ring = (struct rte_mbuf **)rte_zmalloc_socket("rq->mbuf_ring",
-			sizeof(struct rte_mbuf *) * enic->config.rq_desc_count,
+			sizeof(struct rte_mbuf *) * nb_desc,
 			RTE_CACHE_LINE_SIZE, rq->socket_id);
 
 	if (rq->mbuf_ring != NULL)
-- 
2.7.0

Re: [dpdk-dev] [PATCH] enic: don't set enic->config.rq_desc_count in enic_alloc_rq()

[Bruce Richardson]

On Thu, Mar 17, 2016 at 03:46:50PM -0700, John Daley wrote:
> From: Nelson Escobar <neescoba@cisco.com>
> 
> When the requested number of rx descriptors was less than the amount
> configured on the vic, enic_alloc_rq() was incorrectly setting
> enic->config.rq_desc_count to the lower value.  This screwed up later
> calls to enic_alloc_rq().

Can you perhaps clarify what exactly happened when the calls got "screwed up"?
Did the calls just fail, making the queue unusable, or did it crash the application,
or something else. 

Similarly, can you perhaps reword the patch title to start with the word "fix"
as this patch is for a bug-fix, and describe in a couple of words there what
the bug was that was being fixed e.g. "fix rx queue lockup", rather than 
describing the technicalities of the fix. In general, try to avoid using
function or variable names in the titles of messages.

Lastly, can you please include a "Fixes" line in the message, as described here:
http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body

Thanks,
/Bruce

> 
> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> Reviewed-by: John Daley <johndale@cisco.com>
> ---
>  drivers/net/enic/enic_main.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 9fff020..cd7857f 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -524,24 +524,22 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
>  				"policy.  Applying the value in the adapter "\
>  				"policy (%d).\n",
>  				queue_idx, nb_desc, enic->config.rq_desc_count);
> -		} else if (nb_desc != enic->config.rq_desc_count) {
> -			enic->config.rq_desc_count = nb_desc;
> -			dev_info(enic,
> -				"RX Queues - effective number of descs:%d\n",
> -				nb_desc);
> +			nb_desc = enic->config.rq_desc_count;
>  		}
> +		dev_info(enic, "RX Queues - effective number of descs:%d\n",
> +			 nb_desc);
>  	}
>  
>  	/* Allocate queue resources */
>  	rc = vnic_rq_alloc(enic->vdev, rq, queue_idx,
> -		enic->config.rq_desc_count, sizeof(struct rq_enet_desc));
> +		nb_desc, sizeof(struct rq_enet_desc));
>  	if (rc) {
>  		dev_err(enic, "error in allocation of rq\n");
>  		goto err_exit;
>  	}
>  
>  	rc = vnic_cq_alloc(enic->vdev, &enic->cq[queue_idx], queue_idx,
> -		socket_id, enic->config.rq_desc_count,
> +		socket_id, nb_desc,
>  		sizeof(struct cq_enet_rq_desc));
>  	if (rc) {
>  		dev_err(enic, "error in allocation of cq for rq\n");
> @@ -550,7 +548,7 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
>  
>  	/* Allocate the mbuf ring */
>  	rq->mbuf_ring = (struct rte_mbuf **)rte_zmalloc_socket("rq->mbuf_ring",
> -			sizeof(struct rte_mbuf *) * enic->config.rq_desc_count,
> +			sizeof(struct rte_mbuf *) * nb_desc,
>  			RTE_CACHE_LINE_SIZE, rq->socket_id);
>  
>  	if (rq->mbuf_ring != NULL)
> -- 
> 2.7.0
> 

[dpdk-dev] [PATCH v2] enic: fix incorrect setting of rx descriptor limit

[John Daley]

From: Nelson Escobar <neescoba@cisco.com>

On initialization, the rq descriptor count was set to the limit
of the vic.  When the requested number of rx descriptors was
less than this count, enic_alloc_rq() was incorrectly setting
the count to the lower value.  This results in later calls to
enic_alloc_rq() incorrectly using the lower value as the adapter
limit.

Signed-off-by: Nelson Escobar <neescoba@cisco.com>
Fixes: fefed3d1e62c ("enic: new driver")
Reviewed-by: John Daley <johndale@cisco.com>
---
fix checkin comment
 drivers/net/enic/enic_main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 9fff020..cd7857f 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -524,24 +524,22 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 				"policy.  Applying the value in the adapter "\
 				"policy (%d).\n",
 				queue_idx, nb_desc, enic->config.rq_desc_count);
-		} else if (nb_desc != enic->config.rq_desc_count) {
-			enic->config.rq_desc_count = nb_desc;
-			dev_info(enic,
-				"RX Queues - effective number of descs:%d\n",
-				nb_desc);
+			nb_desc = enic->config.rq_desc_count;
 		}
+		dev_info(enic, "RX Queues - effective number of descs:%d\n",
+			 nb_desc);
 	}
 
 	/* Allocate queue resources */
 	rc = vnic_rq_alloc(enic->vdev, rq, queue_idx,
-		enic->config.rq_desc_count, sizeof(struct rq_enet_desc));
+		nb_desc, sizeof(struct rq_enet_desc));
 	if (rc) {
 		dev_err(enic, "error in allocation of rq\n");
 		goto err_exit;
 	}
 
 	rc = vnic_cq_alloc(enic->vdev, &enic->cq[queue_idx], queue_idx,
-		socket_id, enic->config.rq_desc_count,
+		socket_id, nb_desc,
 		sizeof(struct cq_enet_rq_desc));
 	if (rc) {
 		dev_err(enic, "error in allocation of cq for rq\n");
@@ -550,7 +548,7 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 
 	/* Allocate the mbuf ring */
 	rq->mbuf_ring = (struct rte_mbuf **)rte_zmalloc_socket("rq->mbuf_ring",
-			sizeof(struct rte_mbuf *) * enic->config.rq_desc_count,
+			sizeof(struct rte_mbuf *) * nb_desc,
 			RTE_CACHE_LINE_SIZE, rq->socket_id);
 
 	if (rq->mbuf_ring != NULL)
-- 
2.7.0

Re: [dpdk-dev] [PATCH v2] enic: fix incorrect setting of rx descriptor limit

[Bruce Richardson]

On Fri, Mar 18, 2016 at 11:33:34AM -0700, John Daley wrote:
> From: Nelson Escobar <neescoba@cisco.com>
> 
> On initialization, the rq descriptor count was set to the limit
> of the vic.  When the requested number of rx descriptors was
> less than this count, enic_alloc_rq() was incorrectly setting
> the count to the lower value.  This results in later calls to
> enic_alloc_rq() incorrectly using the lower value as the adapter
> limit.
> 
> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> Fixes: fefed3d1e62c ("enic: new driver")
> Reviewed-by: John Daley <johndale@cisco.com>

Please put the fixes line above the signoffs, not in the middle of them. It just
makes it easier to read. I'll fix this for this patch when applying it.

/Bruce

Re: [dpdk-dev] [PATCH v2] enic: fix incorrect setting of rx descriptor limit

[Bruce Richardson]

On Tue, Mar 22, 2016 at 05:32:41PM +0000, Bruce Richardson wrote:
> On Fri, Mar 18, 2016 at 11:33:34AM -0700, John Daley wrote:
> > From: Nelson Escobar <neescoba@cisco.com>
> > 
> > On initialization, the rq descriptor count was set to the limit
> > of the vic.  When the requested number of rx descriptors was
> > less than this count, enic_alloc_rq() was incorrectly setting
> > the count to the lower value.  This results in later calls to
> > enic_alloc_rq() incorrectly using the lower value as the adapter
> > limit.
> > 
> > Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> > Fixes: fefed3d1e62c ("enic: new driver")
> > Reviewed-by: John Daley <johndale@cisco.com>
> 
> Please put the fixes line above the signoffs, not in the middle of them. It just
> makes it easier to read. I'll fix this for this patch when applying it.
> 
> /Bruce

Applied to dpdk-next-net/rel_16_04

/Bruce